Skip to content

Commit e327a7d

Browse files
chubes4chubes4
authored
fix: deduplicate workspace hygiene lock counts (#477)
1 parent 8105010 commit e327a7d

3 files changed

Lines changed: 224 additions & 28 deletions

File tree

‎inc/Workspace/WorkspaceLockStore.php‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,12 +135,14 @@ public static function status(): array {
135135

136136
// phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Table name from $wpdb->prefix, not user input.
137137
return array(
138-
'available' => true,
139-
'table' => $table,
140-
'active' => (int) $wpdb->get_var($wpdb->prepare("SELECT COUNT(*) FROM {$table} WHERE status = %s AND expires_at >= %s", 'active', $now)),
141-
'stale' => (int) $wpdb->get_var($wpdb->prepare("SELECT COUNT(*) FROM {$table} WHERE status = %s AND expires_at < %s", 'active', $now)),
142-
'released' => (int) $wpdb->get_var($wpdb->prepare("SELECT COUNT(*) FROM {$table} WHERE status = %s", 'released')),
143-
'total' => (int) $wpdb->get_var("SELECT COUNT(*) FROM {$table}"),
138+
'available' => true,
139+
'table' => $table,
140+
'active' => (int) $wpdb->get_var($wpdb->prepare("SELECT COUNT(*) FROM {$table} WHERE status = %s AND expires_at >= %s", 'active', $now)),
141+
'active_keys' => self::lock_keys_for_status('active', false),
142+
'stale' => (int) $wpdb->get_var($wpdb->prepare("SELECT COUNT(*) FROM {$table} WHERE status = %s AND expires_at < %s", 'active', $now)),
143+
'stale_keys' => self::lock_keys_for_status('active', true),
144+
'released' => (int) $wpdb->get_var($wpdb->prepare("SELECT COUNT(*) FROM {$table} WHERE status = %s", 'released')),
145+
'total' => (int) $wpdb->get_var("SELECT COUNT(*) FROM {$table}"),
144146
);
145147
// phpcs:enable WordPress.DB.PreparedSQL.InterpolatedNotPrepared
146148
}
@@ -231,6 +233,36 @@ private static function table_name(): string {
231233
return $wpdb->prefix . 'datamachine_code_locks';
232234
}
233235

236+
/**
237+
* Return distinct lock keys for active or expired active DB rows.
238+
*
239+
* @return array<int,string>
240+
*/
241+
private static function lock_keys_for_status( string $status, bool $expired ): array {
242+
global $wpdb;
243+
if ( ! is_object($wpdb) || ! is_callable(array( $wpdb, 'prepare' )) || ! is_callable(array( $wpdb, 'get_col' )) ) {
244+
return array();
245+
}
246+
247+
$table = self::table_name();
248+
$now = gmdate('Y-m-d H:i:s');
249+
$operator = $expired ? '<' : '>=';
250+
251+
// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Table name from $wpdb->prefix, operator is constant-selected above.
252+
$query = call_user_func(array( $wpdb, 'prepare' ), "SELECT DISTINCT lock_key FROM {$table} WHERE status = %s AND expires_at {$operator} %s", $status, $now);
253+
$keys = call_user_func(array( $wpdb, 'get_col' ), $query);
254+
if ( ! is_array($keys) ) {
255+
return array();
256+
}
257+
258+
return array_values(
259+
array_filter(
260+
array_map('strval', $keys),
261+
static fn( string $key ): bool => '' !== $key
262+
)
263+
);
264+
}
265+
234266
private static function expires_seconds(): int {
235267
$seconds = self::DEFAULT_EXPIRES_SECONDS;
236268
if ( function_exists('apply_filters') ) {

‎inc/Workspace/WorkspaceMutationLock.php‎

Lines changed: 62 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,8 @@ public static function status( string $workspace_path ): array {
180180
return array(
181181
'database' => $database,
182182
'filesystem' => $filesystem,
183-
'active' => (int) ( $database['active'] ?? 0 ) + (int) ( $filesystem['active'] ?? 0 ),
184-
'stale' => (int) ( $database['stale'] ?? 0 ) + (int) ( $filesystem['stale'] ?? 0 ),
183+
'active' => self::logical_lock_count($database, $filesystem, 'active'),
184+
'stale' => self::logical_lock_count($database, $filesystem, 'stale'),
185185
'retention_enabled' => true,
186186
'policy' => self::retention_policy(),
187187
);
@@ -216,6 +216,47 @@ private static function sanitize_repo_key( string $repo ): string {
216216
return trim( (string) $repo, '-.');
217217
}
218218

219+
/**
220+
* Count logical locks once when DB rows and flock files describe the same key.
221+
*
222+
* @param array<string,mixed> $database DB lock status.
223+
* @param array<string,mixed> $filesystem Filesystem lock status.
224+
*/
225+
private static function logical_lock_count( array $database, array $filesystem, string $state ): int {
226+
$database_count = (int) ( $database[ $state ] ?? 0 );
227+
$filesystem_count = (int) ( $filesystem[ $state ] ?? 0 );
228+
$database_keys = self::lock_status_keys($database, $state);
229+
$filesystem_keys = self::lock_status_keys($filesystem, $state);
230+
231+
if ( array() === $database_keys && array() === $filesystem_keys ) {
232+
return $database_count + $filesystem_count;
233+
}
234+
235+
$known_count = count(array_unique(array_merge($database_keys, $filesystem_keys)));
236+
$unknown_db = max(0, $database_count - count($database_keys));
237+
$unknown_fs = max(0, $filesystem_count - count($filesystem_keys));
238+
239+
return $known_count + $unknown_db + $unknown_fs;
240+
}
241+
242+
/**
243+
* @param array<string,mixed> $status Lock status.
244+
* @return array<int,string>
245+
*/
246+
private static function lock_status_keys( array $status, string $state ): array {
247+
$keys = $status[ $state . '_keys' ] ?? array();
248+
if ( ! is_array($keys) ) {
249+
return array();
250+
}
251+
252+
return array_values(
253+
array_filter(
254+
array_map('strval', $keys),
255+
static fn( string $key ): bool => '' !== $key
256+
)
257+
);
258+
}
259+
219260
/**
220261
* @return array<string,mixed>
221262
*/
@@ -225,9 +266,11 @@ private static function filesystem_status( string $workspace_path ): array {
225266
$files = false === $files ? array() : $files;
226267
$policy = self::retention_policy();
227268
$cutoff = time() - (int) $policy['filesystem_stale_after_seconds'];
228-
$active = 0;
229-
$stale = 0;
230-
$recent = 0;
269+
$active = 0;
270+
$stale = 0;
271+
$recent = 0;
272+
$active_keys = array();
273+
$stale_keys = array();
231274

232275
foreach ( $files as $file ) {
233276
$handle = fopen($file, 'c'); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fopen
@@ -237,6 +280,7 @@ private static function filesystem_status( string $workspace_path ): array {
237280

238281
if ( ! flock($handle, LOCK_EX | LOCK_NB) ) { // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_flock
239282
++$active;
283+
$active_keys[] = self::lock_key_from_path($file);
240284
fclose($handle); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fclose
241285
continue;
242286
}
@@ -246,20 +290,28 @@ private static function filesystem_status( string $workspace_path ): array {
246290
$mtime = filemtime($file);
247291
if ( false !== $mtime && $mtime < $cutoff ) {
248292
++$stale;
293+
$stale_keys[] = self::lock_key_from_path($file);
249294
} else {
250295
++$recent;
251296
}
252297
}
253298

254299
return array(
255-
'lock_dir' => $lock_dir,
256-
'total' => count($files),
257-
'active' => $active,
258-
'stale' => $stale,
259-
'recent' => $recent,
300+
'lock_dir' => $lock_dir,
301+
'total' => count($files),
302+
'active' => $active,
303+
'active_keys' => array_values(array_filter($active_keys)),
304+
'stale' => $stale,
305+
'stale_keys' => array_values(array_filter($stale_keys)),
306+
'recent' => $recent,
260307
);
261308
}
262309

310+
private static function lock_key_from_path( string $path ): string {
311+
$filename = basename($path);
312+
return str_ends_with($filename, '.lock') ? substr($filename, 0, -5) : $filename;
313+
}
314+
263315
/**
264316
* @return array<string,mixed>
265317
*/

‎tests/smoke-workspace-mutation-lock.php‎

Lines changed: 124 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,15 +51,115 @@ function is_wp_error( $thing ): bool
5151
}
5252
}
5353

54-
if (! function_exists('wp_mkdir_p') ) {
55-
function wp_mkdir_p( string $path ): bool
56-
{
57-
return is_dir($path) || mkdir($path, 0755, true);
58-
}
59-
}
54+
if (! function_exists('wp_mkdir_p') ) {
55+
function wp_mkdir_p( string $path ): bool
56+
{
57+
return is_dir($path) || mkdir($path, 0755, true);
58+
}
59+
}
60+
61+
if (! function_exists('wp_json_encode') ) {
62+
function wp_json_encode( $data, int $flags = 0 )
63+
{
64+
return json_encode($data, $flags);
65+
}
66+
}
67+
68+
class Workspace_Mutation_Lock_Test_Wpdb
69+
{
70+
public string $prefix = 'wp_';
71+
public int $insert_id = 0;
72+
public int $rows_affected = 0;
73+
public string $last_error = '';
74+
75+
/** @var array<int,array<string,mixed>> */
76+
public array $rows = array();
77+
78+
public function insert( string $table, array $data, array $format ): int // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed
79+
{
80+
$this->insert_id++;
81+
$data['id'] = $this->insert_id;
82+
$this->rows[ $this->insert_id ] = $data;
83+
return 1;
84+
}
85+
86+
public function update( string $table, array $data, array $where, array $format, array $where_format ): int // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed
87+
{
88+
$id = (int) ( $where['id'] ?? 0 );
89+
if (! isset($this->rows[ $id ]) ) {
90+
return 0;
91+
}
92+
$this->rows[ $id ] = array_merge($this->rows[ $id ], $data);
93+
return 1;
94+
}
6095

61-
include __DIR__ . '/../inc/Workspace/WorkspaceLockStore.php';
62-
include __DIR__ . '/../inc/Workspace/WorkspaceMutationLock.php';
96+
public function get_var( string $sql ): mixed
97+
{
98+
if (str_contains($sql, 'SHOW TABLES LIKE') ) {
99+
return $this->prefix . 'datamachine_code_locks';
100+
}
101+
102+
if (! str_contains($sql, 'COUNT(*)') ) {
103+
return null;
104+
}
105+
106+
return count($this->matching_rows($sql));
107+
}
108+
109+
/**
110+
* @return array<int,string>
111+
*/
112+
public function get_col( string $sql ): array
113+
{
114+
$keys = array_map(
115+
static fn( array $row ): string => (string) ( $row['lock_key'] ?? '' ),
116+
$this->matching_rows($sql)
117+
);
118+
return array_values(array_unique(array_filter($keys)));
119+
}
120+
121+
public function prepare( string $query, mixed ...$args ): string
122+
{
123+
foreach ( $args as $arg ) {
124+
$replacement = is_int($arg) ? (string) $arg : "'" . str_replace("'", "''", (string) $arg) . "'";
125+
$query = preg_replace('/%[sd]/', $replacement, $query, 1) ?? $query;
126+
}
127+
return $query;
128+
}
129+
130+
/**
131+
* @return array<int,array<string,mixed>>
132+
*/
133+
private function matching_rows( string $sql ): array
134+
{
135+
$now = gmdate('Y-m-d H:i:s');
136+
return array_values(
137+
array_filter(
138+
$this->rows,
139+
static function ( array $row ) use ( $sql, $now ): bool {
140+
$status = (string) ( $row['status'] ?? '' );
141+
$expiry = (string) ( $row['expires_at'] ?? '' );
142+
if (str_contains($sql, "status = 'active'") && 'active' !== $status ) {
143+
return false;
144+
}
145+
if (str_contains($sql, "status = 'released'") ) {
146+
return 'released' === $status;
147+
}
148+
if (str_contains($sql, 'expires_at <') ) {
149+
return '' !== $expiry && $expiry < $now;
150+
}
151+
if (str_contains($sql, 'expires_at >=') ) {
152+
return '' !== $expiry && $expiry >= $now;
153+
}
154+
return true;
155+
}
156+
)
157+
);
158+
}
159+
}
160+
161+
include __DIR__ . '/../inc/Workspace/WorkspaceLockStore.php';
162+
include __DIR__ . '/../inc/Workspace/WorkspaceMutationLock.php';
63163

64164
$failures = 0;
65165
$total = 0;
@@ -109,11 +209,23 @@ function () use ( $tmp ) {
109209
if (! is_wp_error($first) ) {
110210
$first->release();
111211
}
112-
$status = \DataMachineCode\Workspace\WorkspaceMutationLock::status($tmp);
113-
$assert(0, (int) $status['active'], 'released filesystem lock is no longer active');
114-
$assert(2, (int) $status['filesystem']['recent'], 'released filesystem lock files are visible as recent retention residue');
212+
$status = \DataMachineCode\Workspace\WorkspaceMutationLock::status($tmp);
213+
$assert(0, (int) $status['active'], 'released filesystem lock is no longer active');
214+
$assert(2, (int) $status['filesystem']['recent'], 'released filesystem lock files are visible as recent retention residue');
215+
216+
$GLOBALS['wpdb'] = new Workspace_Mutation_Lock_Test_Wpdb();
217+
$db_backed = \DataMachineCode\Workspace\WorkspaceMutationLock::acquire($tmp, 'db-demo', 1);
218+
$assert(false, is_wp_error($db_backed), 'DB-backed acquisition succeeds');
219+
$db_backed_status = \DataMachineCode\Workspace\WorkspaceMutationLock::status($tmp);
220+
$assert(1, (int) $db_backed_status['database']['active'], 'DB-backed acquisition records one active DB row');
221+
$assert(1, (int) $db_backed_status['filesystem']['active'], 'DB-backed acquisition also holds one filesystem lock');
222+
$assert(1, (int) $db_backed_status['active'], 'same DB row and filesystem lock count as one logical active lock');
223+
if (! is_wp_error($db_backed) ) {
224+
$db_backed->release();
225+
}
226+
unset($GLOBALS['wpdb']);
115227

116-
$stale_path = $tmp . '/.locks/worktree-stale.lock';
228+
$stale_path = $tmp . '/.locks/worktree-stale.lock';
117229
touch($stale_path, time() - 172800);
118230
$stale_status = \DataMachineCode\Workspace\WorkspaceMutationLock::status($tmp);
119231
$assert(1, (int) $stale_status['stale'], 'old unlocked filesystem lock is counted stale');

0 commit comments

Comments
 (0)