← All changes | app/Modules/MCP/Support/SlotLock.php +45 -93 2.4.0 → 2.5.0 View file →

		@@ -4,65 +4,39 @@
4	4
5	5	defined('ABSPATH') || exit;
6	6
7	7	/**
8		- * A short-lived exclusive hold on one event/slot/host combination.
	8	+ * A short-lived exclusive hold on one slot for one host.
9	9	*
10		- * Availability is answered by a query and a booking is written by a separate
11		- * INSERT, with the whole slot engine in between. Nothing in the schema stops two
12		- * rows landing on the same host at the same minute — there is no unique index
13		- * over (event, host, start_time), and there cannot be a simple one, because
14		- * group events legitimately seat several bookings in one slot. So "check, then
15		- * write" is a race, and re-checking immediately before the write narrows it
16		- * without closing it.
	10	+ * Checking availability and inserting the booking is a race, and the schema
	11	+ * can't close it: group events seat several bookings in one slot, so there is
	12	+ * no unique index on (event, host, start_time). MCP makes the race likelier
	13	+ * because agents retry and the confirm round-trip adds a human-length pause.
17	14	*
18		- * That race has always existed on the public booking page, where the two
19		- * requests have to arrive within milliseconds of each other. It matters more
20		- * here: an agent retries on its own initiative, several agents can hold
21		- * credentials for the same site, and the confirm round-trip deliberately puts a
22		- * human-length pause between the preview and the write.
	15	+ * The lock is an INSERT IGNORE on the unique `option_name` index, so exactly
	16	+ * one concurrent caller wins. GET_LOCK is avoided: some managed hosts disable
	17	+ * it, and it is per-connection, which connection pooling breaks.
23	18	*
24		- * `add_option()` is the primitive, for the same reason WriteGuard uses it: it
25		- * bottoms out in one INSERT against the unique index on `option_name`, so
26		- * exactly one of N concurrent callers gets `true` back. That is a real mutual
27		- * exclusion, unlike a get-then-set on a transient, and it needs no new table and
28		- * no MySQL-specific advisory lock (`GET_LOCK` is unavailable on some managed
29		- * hosts and is per-connection, which connection pooling makes unreliable).
	19	+ * Not a general-purpose lock: the TTL is seconds, a failed acquire returns
	20	+ * instead of waiting, and an expired lock is stolen.
30	21	*
31		- * Deliberately NOT a general-purpose lock: the TTL is seconds, a failure to
32		- * acquire is reported to the caller rather than waited on, and an expired lock
33		- * is stolen rather than honoured. A booking that cannot be written in fifteen
34		- * seconds has a bigger problem than contention.
	22	+ * Only MCP writes take these locks. Bookings from the public page and admin UI
	23	+ * don't, so against those, availability re-checking is still the only guard.
35	24	*
36		- * WHAT THIS DOES NOT DO, stated plainly so the next reader does not assume more
37		- * than it delivers: acquireInterval() claims every bucket a booking touches, so
38		- * partial overlaps on one host collide across event types. But only MCP writes
39		- * take these locks. The public booking page and the admin UI take none, so a
40		- * booking made there races exactly as it always has, and availability
41		- * re-checking remains the only defence against it.
42		- *
43	25	* @since 2.3.0
44	26	*/
45	27	class SlotLock
46	28	{
47		- /**
48		- * Long enough for a slot query plus an insert and its hooks, short enough
49		- * that a fatal mid-write frees the slot before anyone notices.
50		- */
	29	+ // Covers a slot query plus the insert and its hooks; short enough that a
	30	+ // fatal mid-write frees the slot quickly.
51	31	const TTL = 15;
52	32
53	33	const PREFIX = 'fcal_mcp_slot_';
54	34
55		- /**
56		- * Lock granularity, in seconds. The finest slot interval the plugin offers,
57		- * so a booking aligned to any configurable duration claims whole buckets.
58		- */
	35	+ // Lock granularity in seconds: the finest slot interval the plugin offers.
59	36	const BUCKET = 900;
60	37
61		- /**
62		- * A day of buckets. A booking cannot legitimately need more, and a bad end
63		- * time must not turn into an unbounded row-insert loop.
64		- */
	38	+ // A day of buckets, so a bad end time can't become an unbounded insert loop.
65	39	const MAX_BUCKETS = 96;
66	40
67	41	/**
68	42	* Take the lock for one slot, or return false when someone else holds it.
		@@ -90,12 +64,11 @@
90	64	);
91	65
92	66	$record = ($existing === null) ? null : maybe_unserialize($existing);
93	67
94		- // Steal an expired lock: a request that died mid-write must not hold a
95		- // slot closed until the daily cleanup runs. Conditional on the value
96		- // just read, so of two requests racing the same expired lock the slower
97		- // one cannot delete the winner's fresh row and then insert its own.
	68	+ // Steal an expired lock left by a request that died mid-write. Deleting
	69	+ // by the value just read stops a slower racer from deleting the
	70	+ // winner's fresh row.
98	71	if (is_array($record) && !empty($record['expires']) && $record['expires'] < time()) {
99	72	$wpdb->query( // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching
100	73	$wpdb->prepare(
101	74	"DELETE FROM `{$wpdb->options}` WHERE `option_name` = %s AND `option_value` = %s",
		@@ -106,12 +79,10 @@
106	79
107	80	wp_cache_delete($key, 'options');
108	81	}
109	82
110		- // INSERT IGNORE, the primitive core uses for its own locks
111		- // (WP_Upgrader::create_lock). Deliberately not add_option(): that issues
112		- // ON DUPLICATE KEY UPDATE and reports success to a caller whose row
113		- // already existed, which is no mutual exclusion at all.
	83	+ // Same primitive as WP_Upgrader::create_lock. Not add_option(): its ON
	84	+ // DUPLICATE KEY UPDATE reports success even when the row already existed.
114	85	$inserted = $wpdb->query( // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching
115	86	$wpdb->prepare(
116	87	"INSERT IGNORE INTO `{$wpdb->options}` (`option_name`, `option_value`, `autoload`) VALUES (%s, %s, 'no')",
117	88	$key,
		@@ -132,10 +103,10 @@
132	103	if (!$inserted) {
133	104	return false;
134	105	}
135	106
136		- // The handle carries the owner, so release() can prove it holds this
137		- // lock rather than a successor's.
	107	+ // The owner in the handle lets release() prove it holds this lock and
	108	+ // not a successor's.
138	109	return $key . '|' . $owner;
139	110	}
140	111
141	112	/**
		@@ -140,12 +111,10 @@
140	111
141	112	/**
142	113	* Claim one instant for every host a booking would occupy, all or nothing.
143	114	*
144		- * A collective event books each of its hosts, and two single-host event
145		- * types can share an owner, so the constrained resource is a SET of people
146		- * rather than one. Partial claims are released before returning, so a
147		- * caller never holds half a slot.
	115	+ * A collective event books each of its hosts. Partial claims are released
	116	+ * before returning false.
148	117	*
149	118	* @param int $eventId
150	119	* @param string $startTimeUtc
151	120	* @param array $hostIds
		@@ -174,14 +143,10 @@
174	143	/**
175	144	* Claim every bucket a booking would occupy, for every host it would
176	145	* occupy, all or nothing.
177	146	*
178		- * Keying on the start instant alone let two bookings that overlap without
179		- * sharing a start — 10:00 for thirty minutes and 10:15 for fifteen, through
180		- * different event types — take independent keys and interleave. Overlapping
181		- * intervals always share an instant, so claiming every bucket an interval
182		- * touches makes them collide; intervals that merely abut do not, so a
183		- * booking ending at 10:30 still leaves 10:30 free.
	147	+ * Overlapping bookings with different starts (10:00 for 30 min, 10:15 for
	148	+ * 15) always share a bucket, so they collide. Abutting ones don't.
184	149	*
185	150	* @param int $eventId
186	151	* @param string $startTimeUtc 'Y-m-d H:i:s'
187	152	* @param string $endTimeUtc 'Y-m-d H:i:s'
		@@ -216,10 +181,10 @@
216	181	return $handles;
217	182	}
218	183
219	184	/**
220		- * The bucket starts an interval touches, half open so a booking ending on a
221		- * boundary does not claim the bucket beginning there.
	185	+ * Bucket starts an interval touches. Half open, so a booking ending on a
	186	+ * boundary doesn't claim the next bucket.
222	187	*
223	188	* @return array of 'Y-m-d H:i:s'
224	189	*/
225	190	private static function buckets($startTimeUtc, $endTimeUtc)
		@@ -276,17 +241,14 @@
276	241
277	242	/**
278	243	* Re-assert a lease this caller still owns, pushing its expiry out.
279	244	*
280		- * The lease is taken before isSpotAvailable(), which fans out through
281		- * `fluent_booking/remote_booked_events` to a live FreeBusy call per
282		- * connected calendar per host. On a team event with a cold cache that can
283		- * outrun TTL before a single row is written — and acquire() steals an
284		- * expired lease unconditionally, so the race this class exists to close
285		- * reopens exactly when the check is slow.
	245	+ * isSpotAvailable() can make a live FreeBusy call per calendar per host,
	246	+ * which on a team event can outlast TTL, and acquire() steals expired
	247	+ * leases. Renewing before the write keeps the lock held.
286	248	*
287		- * Conditional on the exact stored bytes, so a caller whose lease was
288		- * already stolen gets false rather than stamping over the new owner.
	249	+ * Matches the exact stored bytes, so a caller whose lease was stolen gets
	250	+ * false instead of overwriting the new owner.
289	251	*
290	252	* @param string|false $handle the value returned by acquire()
291	253	*
292	254	* @return bool whether the caller still holds the lock
		@@ -330,11 +292,10 @@
330	292	);
331	293
332	294	wp_cache_delete($key, 'options');
333	295
334		- // MySQL reports CHANGED rows, not matched ones, so renewing inside the
335		- // same second as the last write is a no-op update and reports zero.
336		- // The row is still ours and still current, which is what was asked.
	296	+ // MySQL counts changed rows, not matched ones, so a renew within the
	297	+ // same second reports zero even though the row is still ours.
337	298	return $updated ? true : ($renewed === $existing);
338	299	}
339	300
340	301	/**
		@@ -339,17 +300,13 @@
339	300
340	301	/**
341	302	* Release a lock this caller actually holds.
342	303	*
343		- * The owner check is the point. A lease can expire while a slow booking hook
344		- * is still running; another request then legitimately takes the slot, and an
345		- * ownerless `delete_option()` from the first request would free the second
346		- * one's lock while it was still working.
	304	+ * A lease can expire during a slow booking hook and be taken by another
	305	+ * request; a plain delete_option() would then free the successor's lock.
	306	+ * The delete matches the exact value checked, so a lapsed owner's delete
	307	+ * matches nothing.
347	308	*
348		- * The check and the delete are two statements, so the delete carries the
349		- * proof with it and matches the exact value checked. A successor's row holds
350		- * a different owner token, so a lapsed owner's delete matches nothing.
351		- *
352	309	* @param string|false $handle the value returned by acquire()
353	310	*/
354	311	public static function release($handle)
355	312	{
		@@ -389,23 +346,18 @@
389	346	wp_cache_delete($key, 'options');
390	347	}
391	348
392	349	/**
393		- * Host is part of the key: on a team event two hosts genuinely can be booked
394		- * for the same minute, and locking the slot across all of them would turn a
395		- * correctness guard into a throughput problem.
	350	+ * Keyed on the host, since the person is the constrained resource: it blocks
	351	+ * one host double-booked via two event types, while two team hosts can
	352	+ * still take the same minute. Round robin has no host until
	353	+ * isSpotAvailable() picks one, so it keys on the event and the caller takes
	354	+ * a host-keyed lock afterwards.
396	355	*
397	356	* @return string
398	357	*/
399	358	private static function key($eventId, $startTimeUtc, $hostId)
400	359	{
401		- // Keyed on the HOST once one is known, not the event: the constrained
402		- // resource is the person, and keying on the event let two requests book
403		- // the same host at one instant through different event types.
404		- //
405		- // Round robin has no host until isSpotAvailable() settles one, so it
406		- // falls back to the event and the caller takes a second, host-keyed
407		- // lock afterwards.
408	360	$scope = $hostId ? 'h' . (int) $hostId : 'e' . (int) $eventId;
409	361
410	362	return self::PREFIX . md5($scope . '|' . $startTimeUtc);
411	363	}