Repository navigation
N°9940 - Setup cache reset races with read-only UI requests producing… - #1038
Conversation
… corrupt expression-cache PHP files
There was a problem hiding this comment.
🟡 Changes recommended
The cache-reset race remains, while some live compilation paths may now skip required invalidation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Reduces cache-reset races during setup compilation.
Changes:
- Removes an early full cache reset.
- Makes opcode/APC cache resets conditional.
File summaries
| File | Review |
|---|---|
setup/sequencers/DataAuditSequencer.php |
Moderate: CopySetupFiles() still calls global ResetAllCaches(), leaving the concurrent-request race unresolved. |
setup/compiler.class.inc.php |
Critical: Inferring invalidation policy from temporary-directory use can leave live backup restores and updates running stale generated models. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ($sTempTargetDir != $sFinalTargetDir) { | ||
| // Move the results to the target directory | ||
| SetupUtils::movedir($sTempTargetDir, $sFinalTargetDir); | ||
| } | ||
|
|
||
| // Reset the opcache since otherwise the PHP "model" files may still be cached !! | ||
| // In case of bad luck (this happens **sometimes** - see N. 550), we may analyze the database structure | ||
| // with the previous datamodel still loaded (in opcode cache) and thus fail to create the new fields | ||
| // Finally the application crashes (because of the missing field) when the cache gets updated | ||
| if (function_exists('opcache_reset')) { | ||
| // Zend opcode cache | ||
| opcache_reset(); | ||
| } | ||
| if (function_exists('apc_clear_cache')) { | ||
| // old style APC | ||
| apc_clear_cache(); | ||
| } else { |
| @@ -67,7 +67,6 @@ public function ExecuteStep($sStep = '', $sInstallComment = null): array | |||
| $aRemovedExtensionCodes = $this->oParams->Get('removed_extensions', []); | |||
| $aSelectedExtensionCodes = $this->oParams->Get('selected_extensions', []); | |||
| $bUseSymbolicLinks = $this->oParams->Get('use_symbolic_links', null) === 'on'; | |||
|
| Filename | Overview |
|---|---|
| setup/compiler.class.inc.php | Restricts global opcode/APC invalidation to direct-target compilation; later model initialization and commit paths retain the required cache resets. |
| setup/sequencers/DataAuditSequencer.php | Removes the pre-compilation build-cache reset; audit model reflection executes in a fresh process and compilation refreshes the build cache directory. |
Sequence Diagram
sequenceDiagram
participant Setup
participant Compiler
participant BuildEnv
participant LiveEnv
participant Cache
Setup->>Compiler: Compile selected model
alt Temporary build directory
Compiler->>BuildEnv: Generate and move files
Note over Compiler,Cache: Skip global opcode/APC reset
else Direct target compilation
Compiler->>LiveEnv: Generate files
Compiler->>Cache: Reset opcode/APC cache
end
Setup->>BuildEnv: Initialize model for installation steps
BuildEnv->>Cache: Reset caches before startup
Setup->>LiveEnv: Commit build environment
LiveEnv->>Cache: Reset build and final caches
Reviews (1): Last reviewed commit: "N°9940 - Setup cache reset races with re..." | Re-trigger Greptile
|
Hey, the problem also happens on iTop 3.2.x, maybe the PR should be merged in support/3.2 ? |
|
Just linking the related issue to this PR: |
Race condition during the cache cleanup and the other users during setup