Replace crond with Supercronic, improve cron logging & backend restart behavior #1302
Conversation
WalkthroughMigrate cron handling from crond to supercronic: rename cron log vars and references, replace init/start scripts, add marker-based backend restart flow, update cron job scripts, healthchecks, frontend log entries, CI path, and devcontainer Dockerfile/setup consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as supercronic\n(start-cron.sh)
participant Job as cron_script.sh
participant Marker as /tmp/backend_restart_pending
participant Entry as entrypoint.sh\n(service monitor)
participant Backend as python3\n(backend)
Note over Cron,Job: scheduled job runs
Cron->>Job: execute cron job
Job->>Job: write timestamped log
Job->>Marker: create marker file (restart requested)
Job->>Backend: killall python3 (guarded)
Job->>Job: atomic cleanup of execution queue
Backend-->>Entry: backend exits
Entry->>Marker: check for marker
alt marker exists
Entry->>Marker: remove marker
Entry->>Entry: re-add start-backend.sh → backend restart
else no marker
Entry->>Entry: handle_exit (exit or continue per debug)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-01T19:02:10.635ZApplied to files:
🪛 LanguageTool.github/copilot-instructions.md[grammar] ~89-~89: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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
♻️ Duplicate comments (1)
.devcontainer/Dockerfile (1)
67-67: Critical: DuplicateLOG_CRONdefinition (flagged in root Dockerfile).This file mirrors the same LOG_CRON duplication issue present in the root Dockerfile at lines 64 and 72. Once the root Dockerfile is corrected and regenerated, this file will be automatically updated.
Also applies to: 75-75
🧹 Nitpick comments (2)
install/production-filesystem/services/scripts/cron_script.sh (1)
17-18: Verify atomic log cleanup handles empty file case correctly.The
grep -vcommand returns exit code 1 when no lines match (i.e., when all lines containcron_restart_backend), which would result in an empty output file. Whilemvshould still work, the script doesn't check the result of the grep operation.Consider adding explicit handling for the grep exit code to make the intent clearer:
# Atomic replacement with temp file grep -v "cron_restart_backend" "${LOG_EXECUTION_QUEUE}" > "${LOG_EXECUTION_QUEUE}.tmp" + RC=$? + # grep returns 0 for matches, 1 for no matches (empty file), both are valid + if [ $RC -eq 0 ] || [ $RC -eq 1 ]; then mv "${LOG_EXECUTION_QUEUE}.tmp" "${LOG_EXECUTION_QUEUE}" + else + rm -f "${LOG_EXECUTION_QUEUE}.tmp" + fiinstall/production-filesystem/services/start-cron.sh (1)
5-5: Minor: Variable name still references old tool name.The variable
crond_pidretains its old name but now holds the supercronic process ID. While functionally correct, this could cause confusion during future maintenance. Consider renaming tocron_pidfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.devcontainer/Dockerfile(4 hunks).devcontainer/scripts/setup.sh(1 hunks)Dockerfile(4 hunks)Dockerfile.debian(1 hunks)back/cron_script.sh(1 hunks)front/php/components/logs_defaults.json(1 hunks)front/php/server/util.php(1 hunks)install/production-filesystem/build/init-cron.sh(1 hunks)install/production-filesystem/build/init-crond.sh(0 hunks)install/production-filesystem/entrypoint.sh(2 hunks)install/production-filesystem/services/config/cron/crontab(1 hunks)install/production-filesystem/services/scripts/cron_script.sh(1 hunks)install/production-filesystem/services/start-cron.sh(2 hunks)
💤 Files with no reviewable changes (1)
- install/production-filesystem/build/init-crond.sh
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-09-20T14:08:44.292Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/stream-logs.sh:5-6
Timestamp: 2025-09-20T14:08:44.292Z
Learning: The .devcontainer/scripts/stream-logs.sh script in NetAlertX is designed as a diagnostic tool for troubleshooting devcontainer startup issues. When log files don't exist, this indicates that the executable/services didn't start properly, which is valuable diagnostic information. Pre-creating missing files would mask this diagnostic behavior.
Applied to files:
.devcontainer/scripts/setup.shDockerfile.debian.devcontainer/Dockerfileinstall/production-filesystem/entrypoint.shDockerfile
📚 Learning: 2025-09-20T14:09:29.159Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
Dockerfile.debian.devcontainer/DockerfileDockerfile
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
Applied to files:
Dockerfile.debian.devcontainer/Dockerfile
📚 Learning: 2025-10-19T01:40:57.095Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template:1-2
Timestamp: 2025-10-19T01:40:57.095Z
Learning: In the NetAlertX repository, .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template is an auto-generated file that is intentionally committed to source control. It cannot be regenerated automatically outside the devcontainer environment and is required for the devcontainer to start, creating a bootstrap dependency.
Applied to files:
Dockerfile.debian.devcontainer/DockerfileDockerfile
📚 Learning: 2025-09-20T14:08:48.256Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:90-96
Timestamp: 2025-09-20T14:08:48.256Z
Learning: In the NetAlertX devcontainer setup, the setup.sh script intentionally removes user_notifications.json from the API directory during development environment initialization to prevent notification clutter that accumulates during container launches and development work.
Applied to files:
Dockerfile.debian
📚 Learning: 2025-09-20T02:56:24.501Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/devcontainer.json:5-8
Timestamp: 2025-09-20T02:56:24.501Z
Learning: In the NetAlertX devcontainer setup, the final .devcontainer/Dockerfile is generated by combining the root Dockerfile with .devcontainer/resources/devcontainer-Dockerfile using the generate-dockerfile.sh script. The devcontainer.json should reference the generated file, not the root Dockerfile.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-10-19T15:29:49.532Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: install/production-filesystem/build/init-nginx.sh:3-3
Timestamp: 2025-10-19T15:29:49.532Z
Learning: In Docker build-time initialization scripts (like those in install/production-filesystem/build/), environment variables set in the Dockerfile do not need explicit validation within the scripts. The Docker build process will fail if required environment variables are undefined, providing implicit validation.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-10-26T17:12:30.237Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/start-php-fpm.sh:25-26
Timestamp: 2025-10-26T17:12:30.237Z
Learning: In NetAlertX's start-php-fpm.sh, PHP-FPM error logging is handled through the PHP-FPM configuration file (PHP_FPM_CONFIG_FILE) rather than through shell redirection. The current redirection pattern (stdout to LOG_APP_PHP_ERRORS, stderr to /dev/stderr) is intentional.
Applied to files:
.devcontainer/DockerfileDockerfile
📚 Learning: 2025-11-01T19:02:10.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1263
File: install/production-filesystem/entrypoint.sh:60-86
Timestamp: 2025-11-01T19:02:10.635Z
Learning: In the NetAlertX project (install/production-filesystem/entrypoint.sh), when fail-fast behavior is adopted for entrypoint checks in the future, be lenient during review because tests will be removed or changed to accommodate that behavior. The current continue-on-failure approach exists to support the existing test suite, but this is expected to change along with corresponding test adjustments.
Applied to files:
install/production-filesystem/entrypoint.sh
📚 Learning: 2025-10-26T16:45:41.247Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/scripts/check-ramdisk.sh:0-0
Timestamp: 2025-10-26T16:45:41.247Z
Learning: In NetAlertX check scripts (install/production-filesystem/services/scripts/check-*.sh), not all checks should exit with non-zero status. Some checks, like check-ramdisk.sh, are warning-only and exit 0 even when issues are detected, allowing the application to start despite suboptimal configuration.
Applied to files:
install/production-filesystem/entrypoint.sh
📚 Learning: 2025-10-26T15:39:36.707Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/scripts/check-first-run-config.sh:0-0
Timestamp: 2025-10-26T15:39:36.707Z
Learning: In NetAlertX startup scripts, critical initialization failures (e.g., unable to create config directory or copy default config files) should exit with non-zero status to fail fast and provide clear error messages, rather than continuing in a broken state.
Applied to files:
install/production-filesystem/entrypoint.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: docker_dev
- GitHub Check: docker-tests
🔇 Additional comments (17)
.devcontainer/scripts/setup.sh (1)
29-29: LGTM! Consistent rename.The identifier update from
LOG_CRONDtoLOG_CRONaligns with the broader migration from crond to supercronic-based cron handling.front/php/server/util.php (1)
277-277: LGTM! Log file reference updated consistently.The allowed log filename update from
'crond.log'to'cron.log'is consistent with the project-wide migration to supercronic.install/production-filesystem/services/config/cron/crontab (1)
4-4: LGTM! Best practice applied.Adding a newline at EOF is a formatting best practice and has no functional impact.
front/php/components/logs_defaults.json (1)
110-114: LGTM! All log references updated consistently.The three log file references (event, fileName, filePath) are updated consistently from
crond.logtocron.log, matching the changes infront/php/server/util.php.Dockerfile.debian (1)
75-75: LGTM! Environment variable updated consistently.The rename from
LOG_CRONDtoLOG_CRONis consistent with the project-wide migration to supercronic-based cron handling.install/production-filesystem/services/start-cron.sh (2)
18-21: Cleanup loop still targets old crond process.This loop kills any leftover
crondprocesses, which may no longer be present in a supercronic-only environment. Consider verifying whether this cleanup is still necessary or if it should target supercronic instead.
26-33: LGTM: Conditional logging options and supercronic invocation.The CRON_OPTS logic correctly defaults to
--quietand switches to--debugwhenNETALERTX_DEBUG=1, enabling appropriate visibility for development without verbose logs in production. The supercronic invocation and log redirection are sound.install/production-filesystem/entrypoint.sh (4)
277-277: LGTM: Service name correctly updated to supercronic.The startup script path and service name are properly swapped from crond to supercronic, aligning with the migration objectives.
318-325: LGTM: Backend restart marker pattern enables controlled restarts.The file-based marker mechanism (
/tmp/backend_restart_pending) allows the Python backend to request a restart without triggering full container shutdown, which is well-suited for containerized environments. The logic correctly removes the service, cleans up the marker, and re-adds the service before continuing.
331-335: LGTM: Debug mode correctly suppresses auto-shutdown on service failure.When
NETALERTX_DEBUG=1, individual service failures now log a warning and continue rather than triggeringhandle_exit. This aligns with the PR's intent to support development/debug mode where services can be restarted independently. Based on learnings, this pattern is expected for devcontainer debugging workflows.
339-339: LGTM: 10-second sleep interval appropriately paces the monitoring loop.The sleep ensures the monitoring loop does not spin excessively while checking service health, reducing CPU overhead.
Dockerfile (3)
82-82: LGTM:SYSTEM_SERVICES_CONFIG_CRONenvironment variable added.The new variable correctly points to the cron configuration directory and aligns with the migration to supercronic.
120-120: LGTM:supercronicpackage added to APK installation.The package is correctly included alongside other system utilities for the containerized environment.
166-166: LGTM: Initialization script correctly swapped frominit-crond.shtoinit-cron.sh.The build-time initialization is now aligned with the supercronic migration.
.devcontainer/Dockerfile (3)
1-1: Note: This file is auto-generated.Per the header, this file is automatically generated by
.devcontainer/scripts/generate-configs.shfrom the rootDockerfileand.devcontainer/resources/devcontainer-Dockerfile. Review comments and fixes should be directed at the rootDockerfile; regeneration will apply those changes here.
123-123: LGTM:supercronicpackage correctly included.The APK package installation is aligned with the root Dockerfile changes.
169-169: LGTM: Initialization script correctly updated toinit-cron.sh.Matches the root Dockerfile's migration to supercronic initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
install/production-filesystem/services/healthcheck.sh (1)
23-28: Update stale comment to reflect supercronic.Line 23's comment still references "crond" but the code now checks for "supercronic". Update the comment for consistency and clarity.
-# 1. Check if crond is running +# 1. Check if supercronic is running if pgrep -f "supercronic" > /dev/null; thentest/docker_tests/test_mount_diagnostics_pytest.py (1)
274-274: Consider refactoring for clarity (optional).The exit code logic is correct but the nested condition is complex. Consider extracting the special case or adding a comment to explain why
active_config+unwritableallows issues but exits with 0.For example:
- expected_exit_code = 1 if expected_issues and not (path_name == "active_config" and scenario_name == "unwritable") else 0 + # active_config unwritable warns but doesn't fail (see lines 539-540) + is_active_config_unwritable = (path_name == "active_config" and scenario_name == "unwritable") + expected_exit_code = 1 if expected_issues and not is_active_config_unwritable else 0Or with an early check:
- expected_exit_code = 1 if expected_issues and not (path_name == "active_config" and scenario_name == "unwritable") else 0 + # active_config unwritable shows warnings but exits 0 (diagnostic tool only warns) + if path_name == "active_config" and scenario_name == "unwritable": + expected_exit_code = 0 # warns but doesn't fail + elif expected_issues: + expected_exit_code = 1 + else: + expected_exit_code = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/docker_tests/configurations/test_results.logis excluded by!**/*.log
📒 Files selected for processing (4)
.github/copilot-instructions.md(1 hunks).github/workflows/code_checks.yml(1 hunks)install/production-filesystem/services/healthcheck.sh(1 hunks)test/docker_tests/test_mount_diagnostics_pytest.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1302
File: back/cron_script.sh:6-18
Timestamp: 2025-11-23T16:05:31.331Z
Learning: The `/back/cron_script.sh` file is legacy code maintained only for compatibility with other components. The active/production version is in `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` directory are typically outside the scope of PRs unless specifically addressing compatibility.
📚 Learning: 2025-11-01T19:02:10.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1263
File: install/production-filesystem/entrypoint.sh:60-86
Timestamp: 2025-11-01T19:02:10.635Z
Learning: In the NetAlertX project (install/production-filesystem/entrypoint.sh), when fail-fast behavior is adopted for entrypoint checks in the future, be lenient during review because tests will be removed or changed to accommodate that behavior. The current continue-on-failure approach exists to support the existing test suite, but this is expected to change along with corresponding test adjustments.
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-11-23T16:05:31.331Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1302
File: back/cron_script.sh:6-18
Timestamp: 2025-11-23T16:05:31.331Z
Learning: The `/back/cron_script.sh` file is legacy code maintained only for compatibility with other components. The active/production version is in `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` directory are typically outside the scope of PRs unless specifically addressing compatibility.
Applied to files:
install/production-filesystem/services/healthcheck.sh
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~89-~89: Ensure spelling is correct
Context: ...r tests tests until you've examined the exisiting testFailures and runTests results. - En...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (2)
install/production-filesystem/services/healthcheck.sh (1)
24-24: Verify supercronic process name matches healthcheck pattern.The healthcheck searches for processes matching
"supercronic"viapgrep -f. This depends on how supercronic is invoked in the container startup flow (e.g., viastart-cron.sh). If the process is wrapped, invoked with a full path, or launched with arguments that change thepsoutput, the pattern may fail to detect it.Confirm that the process will appear as
supercronic(or a variant that matches the pattern) when runningpsin the container..github/workflows/code_checks.yml (1)
98-99: Path migration verified and complete.The new Docker test script path
./test/docker_tests/run_docker_tests.shexists and is correctly referenced. The old script at./run_docker_tests.shhas been removed, and no broken references remain in the codebase.
adamoutler
force-pushed
the
supercronic
branch
from
587bb4c to
35b7e80
Compare
2 participants
Summary
Why
crondwill not function without crontab being owned by root and crond must be given the ability to change its own group. These are not compatible behaviors with the hardened container, or any properly created container for that matter.Follow-ups (options)
Summary by CodeRabbit
Chores
Bug Fixes
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.