fix(backups): support databases running on Swarm workers - #5187
Conversation
dosubot
Bot
added
the
size:XL
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ "$(docker inspect --format '{{.State.Running}}' "$CONTAINER_ID" 2>/dev/null)" != "true" ]; then | ||
| CONTAINER_ID=$(${containerSearch}); |
There was a problem hiding this comment.
If the database task is rescheduled to another Swarm node after discovery, the helper remains pinned to the original node and searches only its local Docker socket, causing the backup to fail with “Container not found” despite an active database task elsewhere in the cluster.
Knowledge Base Used:
There was a problem hiding this comment.
Thanks for catching this. I pushed a bounded retry specifically for this pre-backup relocation race. If the database leaves the selected node, the helper exits with a dedicated preflight code; after full cleanup, Dokploy waits for a different running task and retries once on its new node. The direct path uses the same safe handoff. Dump/upload, package-install, and other worker failures are still non-retryable.
I added regression coverage for pending and stale task discovery, short and full container IDs, relocation success, retry exhaustion, cleanup failures, and direct non-relocation failures. I also verified the flow in an isolated three-node Swarm with PostgreSQL: attempt 1 ran on node A, rediscovery waited while node B was unavailable, and attempt 2 completed the backup on node B.
| Image: BACKUP_WORKER_IMAGE, | ||
| Command: ["/bin/sh", "-c"], | ||
| Args: [ | ||
| `apk add --no-cache bash rclone >/dev/null && exec /bin/bash ${BACKUP_SCRIPT_PATH}`, |
There was a problem hiding this comment.
If a worker cannot reach Alpine package repositories, every worker-path backup fails before its script starts because the temporary service installs Bash and rclone at runtime; using a self-contained helper image would remove this availability dependency and repeated installation cost.
Knowledge Base Used:
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Good point. Installing packages at runtime adds both repository-availability risk and repeated startup cost. A small pinned helper image looks like the better long-term direction, but this repository does not currently have a dedicated backup-worker image and release path.
@Siumauricio, would you prefer that image work to be included in this PR, or should I keep this PR focused on fixing worker-node backups and follow up with the helper image separately? I am happy to adjust either way.
There was a problem hiding this comment.
I’d keep this PR focused on the worker-node backup path and handle the dedicated helper image in a follow-up. The runtime installation is a valid reliability concern, but adding an image build and release path would expand the scope and could delay the functional fix. Please track the helper image separately so the follow-up can replace both apk add dependencies with a pinned, self-contained image.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
dosubot
Bot
added
size:XXL
|
@greptileai please re-review the latest commit, especially the P1 relocation and failure-handling paths. @Siumauricio @narcisonunez, when you have a moment, could you please take a look? I addressed the cross-node relocation race and tightened the case where a worker task is rejected before its container starts: cleanup finishes first, a retry only happens after a different database task is found, and it is limited to one retry. I also sanitized escaping command errors so credential-bearing command details cannot reach deployment-error messages or notifications. The latest local run is clean on Node 24: 99 files / 933 tests, along with the real three-node PostgreSQL relocation scenario. Greptile recommended keeping the dedicated helper image as a separate follow-up; please let me know if you agree or would rather include it here. Happy to adjust anything you would like. |
| if (isPreStartError) { | ||
| try { | ||
| databaseTask = await waitForReplacementServiceTask( | ||
| docker, | ||
| serviceTarget, | ||
| databaseTask.containerId, | ||
| ); | ||
| } catch (replacementError) { | ||
| if (replacementError instanceof ReplacementServiceTaskNotFoundError) { | ||
| throw error; | ||
| } | ||
| throw replacementError; | ||
| } |
There was a problem hiding this comment.
When the backup worker is rejected before creating a container for an unrelated reason such as image pulling, placement, secret mounting, or resource availability, BackupWorkerPreStartError enters replacement-database-task discovery even though the database has not moved, delaying the actual failure by up to five minutes.
Knowledge Base Used: Backups and restore
There was a problem hiding this comment.
Thanks for catching this — the unrelated-rejection delay was a real gap. I pushed 6c078d7 so that, after worker cleanup, Dokploy immediately checks the current database task. If the same container is still running, it returns the original worker rejection right away with no sleep or retry. A different running container is retried immediately, while no running task enters the existing bounded relocation wait.
I added a regression for the unchanged-database case and updated the relocation sequence coverage. The latest focused run is 48/48, and the full Node 24 run is 99 files / 933 tests.
@greptileai please re-review the latest commit.
There was a problem hiding this comment.
Re-reviewed commit 6c078d791. The fix addresses the reported issue: after cleanup, Dokploy re-discovers the current database task, immediately returns the original BackupWorkerPreStartError when the same container is still running, retries immediately when a different running container is found, and only enters the bounded replacement wait when no running task is available. The regression test covers the unchanged-container case and verifies that no sleep occurs. I found no further blocking issues in this change.
There was a problem hiding this comment.
Re-reviewed commit 6c078d791. The fix addresses the reported issue: after cleanup, Dokploy re-discovers the current database task, immediately returns the original BackupWorkerPreStartError when the same container is still running, retries immediately when a different running container is found, and only enters the bounded replacement wait when no running task is available. The added regression coverage verifies the unchanged-container case and that no sleep occurs. I found no further blocking issues in this change.
|
@greptileai please re-review the current head (6c078d7). I addressed the new P1 and replied with the behavior and test details in its thread: #5187 (comment) |
|
@greptile review |
2 participants
Summary
Database backups now work when the database task is running on a Swarm worker instead of the manager.
When no local container is found, Dokploy resolves the active Swarm task and starts a one-shot backup service on that node. This covers PostgreSQL, MySQL, MariaDB, MongoDB, and LibSQL while keeping the existing direct backup path unchanged.
The worker command is stored in a short-lived Docker secret, logs are copied into the existing deployment log, task replacements fail safely, and temporary services and secrets are cleaned up.
Testing
Fixes #3516
Happy to adjust anything based on maintainer feedback.
Greptile Summary
The PR enables database backups when Swarm schedules the database on a worker node, while preserving direct execution for local containers.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the previously reported relocation and unrelated worker-rejection paths are addressed, and the runtime package-installation concern was explicitly deferred.
Reviews (4): Last reviewed commit: "fix(backups): fail fast on unrelated wor..." | Re-trigger Greptile
Context used (3)