Configure raster cache lock waits and add safe retention

[pjeweb]

Raster requests currently give up on a contended cache lock after about 2.85 seconds, even when an upstream fetch can take longer. Add cacheLockTimeout, preserve the existing default, and close lock handles on every exit. Retry directory creation and lock-file open failures within the same budget, including concurrent cache-directory removal and recreation.

Add CachePruner::prune() for scheduled retention of old raster tiles, obsolete namespaces, unused metadata and empty directories. Cleanup skips busy tiles and request locking verifies the metadata inode, so waiting requests remain safe when cleanup removes metadata. Before deleting metadata, cleanup rechecks for tiles under the lock, preserving checkpoints written after its scan by a request that later fails. The reserved mapbox-style-proxy subtree is excluded entirely so shared-cache asset writes can continue safely. Freshness TTL and retention remain separate.

Validation: PHPStan and PHPUnit pass locally (145 tests), including regressions for failed requests checkpointing a new namespace after metadata-only and obsolete-namespace scans, directory removal/recreation during lock acquisition and pruning, bounded persistent directory failures, stale directory stats at both pruning entry points, and pruning during a shared-cache Mapbox asset write. Expiry and checkpoint retention cover named and empty cache namespaces; Mapbox exclusion and concurrent-write tests cover cache roots with zero through three trailing slashes. The race regressions fail before the fixes; retention tests also verify that later cleanup can remove the checkpoint and its metadata. CI static analysis and tests pass on PHP 8.2, 8.3, 8.4 and 8.5; CodeQL passes.

[Copilot]

🟡 Changes recommended

A scan-to-lock race can delete metadata for a newly checkpointed tile, leaving subsequent retention runs unable to remove it.

1 open finding

• Avoid deleting metadata based on stale tile scan · New

What changed in this PR

Adds configurable raster cache lock waits and scheduled retention, keeping retention separate from cache freshness.

Changes:

• Adds cacheLockTimeout with the existing 2.85-second default.
• Introduces inode-checked locking and raster cache pruning.
• Adds concurrency tests and retention documentation.

File	Description
tests/​fixtures/​cache-lock-worker.php	Provides subprocess lock coordination.
tests/​CachePrunerTest.php	Tests retention and busy-tile handling.
tests/​CacheLockTest.php	Tests timeouts, cleanup, and inode replacement.
README.md	Documents lock configuration and retention.
classes/​FileLock.php	Encapsulates timed, inode-checked locking.
classes/​CachePruner.php	Removes expired raster cache files.
classes/​Base.php	Integrates locking and metadata access timestamps.
CHANGELOG.md	Records configuration, retention, and locking changes.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Concurrent directory removal can bypass lock retries, and repeated pruning scans perform unnecessary filesystem checks.

1 open finding

• Match coordinate filenames before checking filesystem types · New

1 resolved since last review

• Avoid deleting metadata based on stale tile scan

Previously missed (1)

In code that hasn't changed since last review

Handle mkdir failures through the bounded retry path

classes/​FileLock.php:26

Utils::mkdirp() can throw before the retry logic runs: mkdir() can fail because the directory exists, then the pruner can remove that empty directory before is_dir() checks it (Utils.php:129–130). The request then fails immediately despite a positive lock timeout. Catch this directory-creation failure and let a missing parent reach the bounded retry path, while still reporting failures when the timeout expires.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Raster cleanup can remove shared Mapbox cache directories during asset writes, causing requests to fail.

1 open finding

• Avoid pruning in-use Mapbox cache directories · New

1 resolved since last review

• Match coordinate filenames before checking filesystem types

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Directory recreation after a failed open can still cause lock acquisition to fail before its configured deadline.

0 open findings

1 resolved since last review

• Avoid pruning in-use Mapbox cache directories

Previously missed (1)

In code that hasn't changed since last review

Retry failed lock opens after directory recreation

classes/​FileLock.php:45

If pruning removes the parent directory before fopen(), another request can recreate it after the open fails but before this is_dir() check. Acquisition then throws despite remaining lock budget, even though reopening could succeed. Retry failed opens until the deadline instead of treating an existing directory as proof of a permanent failure. Add a regression covering removal before the open and recreation before this check.

🧠 Review effort: Balanced

[pjeweb]

Addressed the additional FileLock finding from the latest review summary in dfc058c. Failed lock-file opens now retry until the configured deadline instead of treating a currently existing parent directory as a permanent failure. The directory-race regression now also recreates the parent after fopen fails but before the error is checked; this case reproduced the immediate failure before the fix and now succeeds. Persistent directory failures remain bounded. PHPStan and the full local suite pass (130 tests).

[Copilot]

🔵 Needs a closer look

Multiple trailing slashes bypass the Mapbox exclusion, allowing pruning to disrupt asset writes.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Normalize cache root slashes to prevent Mapbox directory pruning

classes/​CachePruner.php:34

A cache root ending in two or more slashes bypasses the Mapbox exclusion: the iterator preserves extra separators in child paths, while $mapboxCachePath removes them. Pruning then enters mapbox-style-proxy and can remove empty directories during asset writes, causing those writes to fail. Normalize the iterator root as well, and extend the existing exclusion regression to cover // and ///.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[pjeweb]

Addressed the repeated-trailing-slash finding from the latest review summary in f3443c3. The cache root is normalized before constructing both the iterator and the Mapbox exclusion path. Both shared-cache asset-write and subtree-preservation regressions now cover roots ending in zero, one, two, and three slashes. The double/triple-slash cases reproduced the failure before this fix; all eight cases now pass. PHPStan and the full local suite pass (136 tests).

[Copilot]

🔵 Needs a closer look

Retention patterns exclude accepted empty namespaces, leaving tiles unpruned and potentially deleting their metadata prematurely.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Empty cache namespace tiles are not matched or pruned correctly

classes/​CachePruner.php:79

Processor::run() accepts an empty cacheServerName, and Result::checkpointCache() writes it as a filename such as 0--0. Both this pattern and the pattern in hasTiles() require a nonempty namespace (.+), so these tiles are never pruned. The under-lock recheck also misses them and can delete old metadata despite a remaining fresh checkpoint. Change the namespace portion to .* in both patterns, and add regression coverage for expired empty-namespace tiles and fresh checkpoints with old metadata.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[pjeweb]

Addressed the accepted-empty-namespace finding from the latest review summary in 61a9902. Both tile patterns now accept an empty namespace, matching Processor and Result behavior. Expiry, fresh-tile retention, and both failed-request stale-scan regressions now cover named and empty namespaces. All four added cases failed before this fix and now pass, including later removal of an aged empty-namespace checkpoint and its metadata. PHPStan and the full local suite pass (140 tests).

[Copilot]

🔵 Needs a closer look

Destructive cleanup and cross-process locking warrant human verification, particularly with the unresolved concurrent-directory-removal race.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Clear stat cache before checking directory existence

classes/​CachePruner.php:65

If another pruner removes this directory after the outer isDir() check, DirectoryIterator throws, but is_dir($path) can still return the cached positive stat result. This rethrows the exception and aborts cleanup instead of treating concurrent removal as a no-op. Clear the stat cache for $path before checking whether the directory still exists.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[pjeweb]

Confirmed and fixed the cached-directory-status finding from the latest review summary in 798068c. The DirectoryIterator exception path now clears the stat cache before checking whether another pruner removed the directory. The regression coordinates a real subprocess through signals and process-status polling, preserving the stale positive stat result; it reproduces the cleanup exception before the fix and now returns zero deletions without aborting. Pipe reads/writes clear the stat cache and would mask this race. PHPStan and the full local suite pass (141 tests).

[Copilot]

🔵 Needs a closer look

Filesystem deletion and lock-replacement guarantees need human review, with a directory-recreation failure still unresolved.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Retry opening directory after transient removal before rethrowing

classes/​CachePruner.php:69

If another pruner removes this directory before DirectoryIterator opens it, and a request recreates it before is_dir() runs, this check rethrows the stale missing-directory exception. That aborts the entire retention run despite the directory being available again. Retry opening it before propagating the error, while still reporting persistent failures. Add a regression covering recreation between the failed open and this check.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[pjeweb]

Addressed the directory-recreation finding from the latest review summary in 2ec29cc, and the related cache-root removal race in f31af33. Root initialization and directory sweeps now share bounded open recovery: missing directories return zero, restored directories are retried once, and persistent open failures are reported. Regressions recreate the directory after a real failed open for both iterator types; both reproduced the stale-exception behavior before recovery was added and now succeed. Subprocess removal coverage also exercises both entry points. PHPStan and the full local suite pass (145 tests).

[Copilot]

🔵 Needs a closer look

Destructive cleanup coordinated through cross-process inode locking warrants final human validation despite the targeted regression coverage.

0 open findings

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.