fix: client updater - #1632
Conversation
📝 WalkthroughWalkthroughAdds a public Updater.changeUrl() UI and restart flow, strengthens client-side validation and gating, guards a missing UI element, and hardens the server updater API with stricter input checks, file locking, atomic cache writes, checksum population, and explicit response headers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Updater (Lua)
participant Server as API (PHP)
participant FS as Cache/Filesystem
User->>Client: Open change-URL dialog (Updater.changeUrl)
Client->>User: Show dialog (OK / Restart)
User->>Client: Confirm new URL
Client->>Client: Cancel/stop current ops, destroy updater windows
Client->>Server: Request update metadata using new URL
Server->>FS: Verify cache directory exists
Server->>FS: Acquire lock (wait/retry if locked)
alt Cache missing or stale
Server->>FS: Compute checksums, write temp cache file
Server->>FS: Atomic rename temp -> cache
end
Server->>Server: Select binary entry by basename
Server->>Client: Return payload with explicit headers
Server->>FS: Release lock
Client->>Client: Restart updater flow with new URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@modules/updater/updater.lua`:
- Around line 296-303: The Cancel button currently calls Updater.check(), which
restarts the entire update and discards progress; change this to resume behavior
by calling an existing resume/continue method (e.g., Updater.resume() or
Updater.continue()) instead of Updater.check(), or if no resume API exists
implement one on the Updater that picks up from saved state and continues the
download/apply process; alternatively, if restart is the intended behavior,
update the button text and comment to "Restart" and keep calling
Updater.check(); locate the button handler (cancelButton.onClick), the dialog
teardown (dialog:destroy), and the Updater methods to apply the appropriate
change.
- Around line 245-247: The comment above the changeUrl function flags it as
AI-generated and untested for mobile — remove the AI-disclosure comment and
instead add a clear TODO and gating: add a TODO/TICKET reference above the
function (changeUrl) stating “mobile testing required” and either wrap the
function activation behind a feature flag/guard (check a config flag like
enableChangeUrl or runtime detection) or disable wiring from the UI until
verified; also update the UI wiring in updater.otui (changeUrlButton) to only
call changeUrl when the flag is enabled and create a short manual test checklist
or ticket to verify behavior on mobile clients.
In `@tools/api/updater.php`:
- Around line 48-86: The cache-building path currently proceeds without holding
the lock when non-blocking flock() fails; change the logic so that when
acquiring the lock via $lock (created from $lock_file) fails with LOCK_EX |
LOCK_NB you wait/retry and then acquire a blocking flock() before proceeding to
build the cache; ensure $lock remains open for the entire cache creation/atomic
write (file_put_contents(... ".tmp") + rename(...)) and only call flock($lock,
LOCK_UN) and fclose($lock) after the rename, and guard any flock/ fclose calls
with an isset(is_resource($lock)) check to avoid unlocking/closing an
already-closed resource.
🧹 Nitpick comments (3)
tools/api/updater.php (1)
63-76: Inconsistent indentation within the foreach block.Lines 64-66 have different indentation than the surrounding new code (lines 70-76). Consider aligning for consistency.
modules/updater/updater.lua (2)
209-215: Consider simplifying the triple-negation condition for readability.The logic is correct but
(not g_platform.isMobile() or not allowCustomServers or not loadModulesFunction)requires mental gymnastics. Consider extracting to a descriptive variable.♻️ Suggested refactor for clarity
local allowCustomServers = ALLOW_CUSTOM_SERVERS or false + local canWaitForCustomUrl = g_platform.isMobile() and allowCustomServers and loadModulesFunction local function progressUpdater(value) removeEvent(scheduledEvent) if value == 100 then return Updater.error(tr("Timeout")) end - if updateData and (value > 60 or (not g_platform.isMobile() or not allowCustomServers or not loadModulesFunction)) then -- gives 3s to set custom updater for mobile version + if updateData and (value > 60 or not canWaitForCustomUrl) then -- gives 6s to set custom updater for mobile version return updateFiles(updateData) end
282-294: Consider basic URL format validation.The handler only checks length > 4, allowing invalid URLs like "aaaa" that will fail on the subsequent
Updater.check(). A simple protocol prefix check would provide earlier feedback to users.♻️ Suggested validation improvement
okButton.onClick = function() local newUrl = textEdit:getText() - if newUrl and newUrl:len() > 4 then + if newUrl and (newUrl:find("^https?://") ~= nil) then Services.updater = newUrl dialog:destroy()
| // File locking to prevent race conditions | ||
| $lock_file = $cache_file . ".lock"; | ||
| $lock = fopen($lock_file, "w"); | ||
| if (!flock($lock, LOCK_EX | LOCK_NB)) { | ||
| // Another process is updating cache, wait and use existing cache | ||
| fclose($lock); | ||
| usleep(100000); // 100ms | ||
| if (file_exists($cache_file)) { | ||
| $cache = json_decode(file_get_contents($cache_file), true); | ||
| } | ||
| } | ||
|
|
||
| if (!$cache) { | ||
| $rii = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir)); | ||
| $cache = array(); | ||
| foreach ($rii as $file) { | ||
| if (!$file->isFile()) | ||
| continue; | ||
| $path = str_replace($dir, '', $file->getPathname()); | ||
| $path = str_replace(DIRECTORY_SEPARATOR, '/', $path); | ||
| $checksum = hash_file("crc32b", $file->getPathname()); | ||
|
|
||
| if ($checksum === true || $checksum != "") { | ||
| $parsed_checksum = ltrim($checksum, '0'); | ||
| if ($parsed_checksum === '') { | ||
| $parsed_checksum = '0'; | ||
| if ($checksum !== false && $checksum !== "") { | ||
| $parsed_checksum = ltrim($checksum, '0'); | ||
| if ($parsed_checksum === '') { | ||
| $parsed_checksum = '0'; | ||
| } | ||
| $cache[$path] = $parsed_checksum; | ||
| } | ||
| $cache[$path] = $parsed_checksum; | ||
| } | ||
| file_put_contents($cache_file . ".tmp", json_encode($cache)); | ||
| rename($cache_file . ".tmp", $cache_file); | ||
|
|
||
| // Release lock | ||
| flock($lock, LOCK_UN); | ||
| } | ||
| if (isset($lock) && is_resource($lock)) { | ||
| fclose($lock); | ||
| } |
There was a problem hiding this comment.
Race condition: cache can be built without lock protection.
When flock() fails to acquire the lock (another process is updating), the code closes $lock on line 53, waits 100ms, then tries to read the cache. If the cache still doesn't exist at line 60, it proceeds to build the cache without holding any lock, defeating the locking mechanism.
Additionally, if this path is taken and cache is built, line 82 calls flock($lock, LOCK_UN) on an already-closed resource.
🐛 Proposed fix to maintain lock throughout cache building
// File locking to prevent race conditions
$lock_file = $cache_file . ".lock";
$lock = fopen($lock_file, "w");
if (!flock($lock, LOCK_EX | LOCK_NB)) {
// Another process is updating cache, wait and use existing cache
- fclose($lock);
usleep(100000); // 100ms
if (file_exists($cache_file)) {
$cache = json_decode(file_get_contents($cache_file), true);
}
+ if (!$cache) {
+ // Wait for the lock if cache still not available
+ flock($lock, LOCK_EX);
+ // Re-check cache after acquiring lock
+ if (file_exists($cache_file)) {
+ $cache = json_decode(file_get_contents($cache_file), true);
+ }
+ }
}
if (!$cache) {
$rii = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir));
$cache = array();
foreach ($rii as $file) {
// ... rest of cache building ...
}
file_put_contents($cache_file . ".tmp", json_encode($cache));
rename($cache_file . ".tmp", $cache_file);
-
- // Release lock
- flock($lock, LOCK_UN);
}
- if (isset($lock) && is_resource($lock)) {
- fclose($lock);
- }
+ // Release lock and close handle
+ flock($lock, LOCK_UN);
+ fclose($lock);
}🤖 Prompt for AI Agents
In `@tools/api/updater.php` around lines 48 - 86, The cache-building path
currently proceeds without holding the lock when non-blocking flock() fails;
change the logic so that when acquiring the lock via $lock (created from
$lock_file) fails with LOCK_EX | LOCK_NB you wait/retry and then acquire a
blocking flock() before proceeding to build the cache; ensure $lock remains open
for the entire cache creation/atomic write (file_put_contents(... ".tmp") +
rename(...)) and only call flock($lock, LOCK_UN) and fclose($lock) after the
rename, and guard any flock/ fclose calls with an isset(is_resource($lock))
check to avoid unlocking/closing an already-closed resource.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/updater/updater.lua (1)
109-115:⚠️ Potential issue | 🟡 MinorAdd checksum type validation to prevent download failures with malformed updater responses.
The condition on line 109-111 validates
data.binary.filebut notdata.binary.checksum. If the updater API returns a binary entry with missing or empty checksum, line 113 will enqueue it with a nil/empty checksum. This causes the download validation at line 44 to fail with a misleading error message ("Should be nil/empty, is: [actual_checksum]"), triggering retries up tomaxRetriesbefore displaying an error dialog. The binary update should instead be silently skipped when the checksum is invalid. Add validation matching thedata.binary.filecheck:🛠️ Suggested fix
- if type(data.binary) == "table" and type(data.binary.file) == "string" and data.binary.file:len() > 1 then + if type(data.binary) == "table" + and type(data.binary.file) == "string" and data.binary.file:len() > 1 + and type(data.binary.checksum) == "string" and data.binary.checksum:len() > 0 then
🤖 Fix all issues with AI agents
In `@modules/updater/updater.lua`:
- Around line 245-309: In Updater.changeUrl(), don't cancel the current update
(removeEvent(scheduledEvent) and HTTP.cancel(httpOperationId)) before showing
the dialog; instead move those cancellations into the okButton.onClick and
restartButton.onClick handlers so they only occur when the user actually
confirms restart/change; additionally, when saving Services.updater (in
okButton.onClick and restartButton.onClick if applicable) normalize the newUrl
to ensure it ends with a trailing slash (append "/" if missing) so subsequent
concatenation like url .. file produces valid paths; reference:
Updater.changeUrl, scheduledEvent, httpOperationId, Services.updater,
okButton.onClick, restartButton.onClick.
|
Tell Opus claude that the problem is NOT in .php or .lua. The problem is in cpp |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/updater/updater.lua (1)
109-115:⚠️ Potential issue | 🟡 MinorValidate
data.binary.checksumbefore use.If
data.binary.checksumis missing or non-string,downloadFiles()will build an error message with.. file_checksumand throw. Guard the checksum to avoid a runtime error on malformed API data.🛡️ Suggested guard
- if type(data.binary) == "table" and type(data.binary.file) == "string" and data.binary.file:len() > 1 then + if type(data.binary) == "table" + and type(data.binary.file) == "string" and data.binary.file:len() > 1 + and type(data.binary.checksum) == "string" and data.binary.checksum:len() > 0 then
🤖 Fix all issues with AI agents
In `@modules/updater/updater.lua`:
- Around line 299-309: The Restart button handler cancels the HTTP op but then
calls Updater.check() which returns early while updaterWindow is still set;
modify the restartButton.onClick to mirror the OK-path teardown by ensuring the
existing updaterWindow is destroyed/cleared before calling Updater.check():
removeEvent(scheduledEvent) and HTTP.cancel(httpOperationId) as already done,
then call dialog:destroy() and also clear the global/updaterWindow reference (or
call the same cleanup used by the OK button) so updaterWindow is nil, and only
then call Updater.check() so the restart actually proceeds.
|
This PR is stale because it has been open 45 days with no activity. |
|
This PR is stale because it has been open 45 days with no activity. |
|
This PR is stale because it has been open 45 days with no activity. |
dudantas
left a comment
dudantas
left a comment
There was a problem hiding this comment.
Hi, thanks for your PR and your time.
The reported updater freeze is still not addressed by this change.
Updater.updateFiles() calls g_resources.filesChecksums() without a file list. The C++ implementation recursively enumerates and reads every file under the virtual root ("/") on the UI thread before downloads start. When running from a source tree, this may include build artifacts, vcpkg_installed, and other files that are not part of the update manifest, making the client appear frozen and delaying the download phase.
The PHP/Lua changes may improve API robustness, but they do not change this checksum path. The client should compute checksums only for the files returned in data.files (or the configured updater roots), rather than scanning the whole resource tree. This is also tracked in #1041.
Please either include the C++/Lua checksum-scope fix in this PR, or narrow the PR description to an independently reproducible API issue that this patch actually resolves.
Hi, thanks for your reply. |
…checksum calculation
|
@dudantas |
4 participants
Description
Fix for Client Auto Updater. The client updater was not working properly, which blocked downloads.
Behavior
Actual
The client couldn't compare files or download the updates from the server.
Expected
The Client should download the files and update successfully.
Type of change
Please delete options that are not relevant.
How Has This Been Tested
The files were changed using Claude Opus 4.5 and may contains some errors. I have no experience with PHP neither this kind of file transfer, so I used AI to generate the fix.
The changes were tested on a nginx server and everything seems OK.
Android Version need better tests.
Test Configuration:
Client - Windows 11
Server - Ubuntu 24.04
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Stability