fix: improve DB retry logic and prevent install loop + OOM handling (…#1610
fix: improve DB retry logic and prevent install loop + OOM handling (…#1610johnny603 wants to merge 2 commits into
Conversation
|
@johnny603 You have these ideas in the description:
I think it would be valuable for OED to look into these. Would you like to open issue(s) about this? Any additional information on your thoughts are also very welcome. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@johnny603 Thank you for thinking about this problem and addressing it. I have not done a complete review but have looked at this some. I probably have other comments but wanted to start here to begin the conversation. I've made some comments on this file to consider. In the bigger picture, where I try to start OED using docker compose up, it fails to start and goes into a loop where it keeps reporting this error:
Warning: Accessing non-existent property 'sqlFile' of module exports inside circular dependency
I'm assuming you don't see this. Do you have thoughts on what is happening?
Please let me know if anything is not clear, any thoughts or questions.
| context: ./ | ||
| dockerfile: ./containers/web/Dockerfile | ||
|
|
||
| environment: |
There was a problem hiding this comment.
I'm wondering about moving the environment variable location in this file. Is this your preference, a good idea or something else. I'm thinking about this because the OED site installation directions talk about these so moving them does have some pain threshold.
| @@ -1,198 +1,120 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
The copyright notice was removed. OED wants that in every file that is possible.
Also, you removed or modified a comments in this file. I'm wondering why since some may have changed the meaning or removed comments designed to describe what was intended.
| exit 1 | ||
| esac | ||
| case "$1" in | ||
| --production) production=yes ;; |
There was a problem hiding this comment.
The indenting was changed from tabs to spaces. While OED is not always consistent, esp. in script files, the project does prefer tab indenting when that is possible. Is there are reason not to use tab indenting?
| esac | ||
| case "$1" in | ||
| --production) production=yes ;; | ||
| --nostart) dostart=no ;; |
There was a problem hiding this comment.
While your new formatting is okay, OED prefers each line of code on a separate line and having items inside a loop/logic indented on separate lines (so it is clear the association).
| # ---------------------------------------------------- | ||
| # Load env | ||
| # ---------------------------------------------------- | ||
| [ -f ".env" ] && source .env |
There was a problem hiding this comment.
While this is the same, OED prefers the explicit if/else to make the logic clear. This is esp. true as we work with a lot of student developers who may not be a familiar with scripting syntax.
| # ---------------------------------------------------- | ||
| # NPM install | ||
| # ---------------------------------------------------- | ||
| if [ "$keep_node_modules" = "yes" ]; then |
There was a problem hiding this comment.
I'm wondering why the logic about changed package files was removed. It was designed only do the install when package files were changed since OED only wants an update then and not every time it is run to keep all the file standard across implementations. This seems to run every time. Thoughts?
| exit 2 | ||
| fi | ||
| fi | ||
| echo "Running database creation..." |
There was a problem hiding this comment.
The script was using printf throughout to be consistent. It also did extra lines to make it easier to see certain items-steps. Could you explain why this was changed?
| # ---------------------------------------------------- | ||
| if [ "$production" = "yes" ] || [ "$OED_PRODUCTION" = "yes" ]; then | ||
| npm run webpack:build | ||
| elif [ "$dostart" = "no" ]; then |
There was a problem hiding this comment.
You systematically changed the == to = for string comparisons. While they are the aliases for each other and = is POSIX compliant (we are tied to bash so less of an issue), the == was used to be explicit it is a string comparison. The reuse of = in other languages has led to many bugs (such as in C++ where you assign instead of just compare) so it was felt that being explicit was better here. What do you think?
2 participants
Description
This change addresses non-deterministic database initialization during Docker startup that results in repeated PromiseRejectionHandledWarning logs, overlapping retry behavior, and inconsistent startup state.
The root issue is caused by two independent retry/initialization systems running concurrently:
installOED.sh retrying npm run createdb
Node.js createDB.js performing async schema initialization without deterministic process termination
Together, these create race conditions, repeated schema execution attempts, and noisy startup logs.
This change improves startup reliability by clarifying DB initialization flow and reducing overlapping execution paths.
Fixes #1608
Type of change
Checklist
ideas
and each author is listed in the Description section.
Limitations
This fix improves initialization determinism and reduces retry-related race conditions, but does not fully eliminate underlying circular dependency complexity in the model layer (models/database.js).
Further improvements may include:
Refactoring schema initialization into a single orchestration layer
Reducing circular dependencies between model modules
Migrating DB initialization to a fully centralized startup service