feat: (critical) implement startup stability fixes for docker (increase timeout)#11154
feat: (critical) implement startup stability fixes for docker (increase timeout)#11154khoinp1012 wants to merge 1 commit intoappwrite:mainfrom
Conversation
📝 WalkthroughWalkthroughTwo files were modified to improve database setup resilience and connection reliability. The HTTP initialization now wraps per-collection creation logic in a try-catch block to gracefully handle exceptions during collection preparation, logging warnings instead of failing the entire setup process. Simultaneously, the MySQL/MariaDB connection timeout was increased from 3 seconds to 10 seconds in the initialization registers. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/init/registers.php`:
- Line 267: The CLI/worker db resource still uses SQL::getPDOAttributes() which
returns a 3s timeout; update the db registration to use the same 10s timeout as
the pool by either changing SQL::getPDOAttributes() to return PDO::ATTR_TIMEOUT
=> 10 or overriding the attributes in the db resource registration (the closure
that creates the PDO used by the "db" service) to set \PDO::ATTR_TIMEOUT => 10
so worker/CLI connections match the pool.
🧹 Nitpick comments (1)
app/http.php (1)
212-242: Good resilience improvement for startup stability.The try-catch block correctly prevents individual collection creation failures from crashing the entire server startup. The warning log maintains visibility into issues.
Two minor observations:
- Consider catching more specific exceptions: The
Migration.phpreference catchesDuplicateExceptionspecifically. You could consider catchingDuplicateExceptionseparately for cleaner logging (similar to line 393), while still catching\Exceptionas a fallback:♻️ Optional: More specific exception handling
try { if (!$database->getCollection($key)->isEmpty()) { continue; } // ... attribute/index mapping ... $database->createCollection($key, $attributes, $indexes); + } catch (DuplicateException $e) { + Console::info(" └── Skip: collection '{$key}' already exists"); } catch (\Exception $e) { Console::warning(" └── Failed to create collection '{$key}': " . $e->getMessage()); }
- Potential inconsistency: The
sharedTablesV2collection creation loop (lines 402-416) doesn't have similar try-catch protection. Consider whether it should also be wrapped for consistency.
| return new PDOProxy(function () use ($dsnHost, $dsnPort, $dsnUser, $dsnPass, $dsnDatabase) { | ||
| return new PDO("mysql:host={$dsnHost};port={$dsnPort};dbname={$dsnDatabase};charset=utf8mb4", $dsnUser, $dsnPass, [ | ||
| \PDO::ATTR_TIMEOUT => 3, // Seconds | ||
| \PDO::ATTR_TIMEOUT => 10, // Seconds |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its size
find . -name "registers.php" -type f | head -20Repository: appwrite/appwrite
Length of output: 85
🏁 Script executed:
# Check what SQL::getPDOAttributes() returns
rg "getPDOAttributes" -A 30 --type phpRepository: appwrite/appwrite
Length of output: 1711
🏁 Script executed:
# Read the specific lines mentioned in the review
if [ -f "app/init/registers.php" ]; then
echo "=== Lines 260-275 (around line 267) ===";
sed -n '260,275p' app/init/registers.php | cat -n;
echo "";
echo "=== Lines 325-350 (around lines 330-344) ===";
sed -n '325,350p' app/init/registers.php | cat -n;
fiRepository: appwrite/appwrite
Length of output: 2086
🏁 Script executed:
# Find the SQL class definition
find . -name "*.php" -type f | xargs rg "class SQL" | head -5Repository: appwrite/appwrite
Length of output: 554
🏁 Script executed:
# Search for getPDOAttributes function definition
rg "function getPDOAttributes\(\)" -A 20 --type phpRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Search in the vendor directory for getPDOAttributes
rg "getPDOAttributes" vendor/utopia-php/database/src/Database/Adapter/SQL.php -A 20Repository: appwrite/appwrite
Length of output: 901
🏁 Script executed:
# Also check the file directly
cat vendor/utopia-php/database/src/Database/Adapter/SQL.php | grep -A 20 "getPDOAttributes"Repository: appwrite/appwrite
Length of output: 901
Timeout increase incomplete: Workers/CLI connections still use 3-second timeout.
The connection pool now uses a 10-second timeout, but the db resource at lines 330-344 (which handles CLI/worker connections) relies on SQL::getPDOAttributes() from the vendor library, which still returns a 3-second timeout. This creates an inconsistency where workers/CLI may timeout before the main connection pool.
Either update SQL::getPDOAttributes() to return 10 seconds, or explicitly set the timeout in the db resource registration to match the connection pool timeout.
🤖 Prompt for AI Agents
In `@app/init/registers.php` at line 267, The CLI/worker db resource still uses
SQL::getPDOAttributes() which returns a 3s timeout; update the db registration
to use the same 10s timeout as the pool by either changing
SQL::getPDOAttributes() to return PDO::ATTR_TIMEOUT => 10 or overriding the
attributes in the db resource registration (the closure that creates the PDO
used by the "db" service) to set \PDO::ATTR_TIMEOUT => 10 so worker/CLI
connections match the pool.
1 participant
These are simple but very critical fixes:
1. Increased Database Connection Timeout (Primary Fix)
File:
app/init/registers.phpPDO::ATTR_TIMEOUTfrom 3 seconds to 10 seconds2. Resilient Collection Initialization (Safety Net)
File:
app/http.phptry-catchblock🧪 Verification
Negative Test (Before Fix)
mainbranch codePositive Test (After Fix)
Server started successfully (max payload is 12,582,912 bytes)