fix(deploy): notify on container crash loop after deployment#72
Open
AminDhouib wants to merge 3 commits into
Open
fix(deploy): notify on container crash loop after deployment#72AminDhouib wants to merge 3 commits into
AminDhouib wants to merge 3 commits into
Conversation
(cherry picked from commit e1a1f44)
- application.command.test.ts: stub waitForSwarmServiceStable (partial mock, rest of docker/utils stays real) — command-generation tests have no real swarm service to poll, so the new 60s stability window timed them out. - application.real.test.ts: raise REAL_TEST_TIMEOUT 180s -> 300s to absorb the fixed 60s stability window each successful real deploy now includes. Fork-side adaptation for the port of Dokploy#4357 (upstream's own pr-check(test) was red for exactly this reason).
e10c24e to
20cb050
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports Dokploy#4357 by @sajdakabir (1 commit, cherry-picked with
-x, original authorship preserved) + 1 fork-side test-adaptation commit.Addresses Dokploy#4339 (verified: linked via the upstream PR's closing references; the diff makes a post-deploy crash loop surface as a failed deployment + build-error notification instead of a silent "done").
What / why
Previously a deployment was marked
done(and success notifications sent) as soon as the container was (re)created — if the app then crash-looped, nothing was reported.waitForSwarmServiceStable(appName, { serverId, windowMs = 60s, pollMs = 5s })inpackages/server/src/utils/docker/utils.ts: polls the swarm service's tasks; latest taskfailed/rejected, or a restart after having been running, → unstable (with reason).deployApplication/rebuildApplicationrun this check aftermechanizeDockerContainer; instability throws, which routes into the existing error path: deployment/application markederrorandsendBuildErrorNotificationsfires (existing payloads only — no DB schema change, no migration).Reviewer heads-up: by upstream design, every successful application deploy now includes a fixed 60s stability window before being marked
done.Adaptations (fork-side, test-only)
Upstream PR Dokploy#4357's own
pr-check (test)is red; reproduced locally — the stability poll hits a real Docker socket from unit tests:__test__/deploy/application.command.test.ts: partial mock ofdocker/utilsstubbingwaitForSwarmServiceStable→{ stable: true }(rest of the module stays real). Without it, 4 command-generation tests time out (5s limit vs 60s poll) — exactly upstream's CI failure.__test__/deploy/application.real.test.ts:REAL_TEST_TIMEOUT180s → 300s, since each successful real deploy now includes the fixed 60s window on top of real nixpacks/railpack build time.No production-code deviation from upstream.
Test evidence (Windows local; Linux CI is source of truth)
__test__/deploy/suite: 52 passed / 1 skipped;application.command.test.ts6/6 (the 4 pre-adaptation timeouts are gone). Onlyapplication.real.test.tsfails locally — inbeforeEachshell setup, pre-existing Windows-local behavior for this Linux-only file, unrelated to this change.pnpm --filter=@dokploy/server run typecheckandpnpm --filter=dokploy run typecheck: clean.Sibling / merge-order notes
packages/server/src/services/application.ts. Hunk analysis: fix: prevent SSH private key from leaking into build-failure notifications #63 edits the catch block (~L250) and the./deploymentimport; this PR edits the try block (~L222/L323) and thedocker/utilsimport — change-disjoint, either merge order is clean; re-run CI on whichever merges second. Once both land, a crash-loop failure's notification message flows through fix: prevent SSH private key from leaking into build-failure notifications #63's log-tail sanitizer, which is the desired combined behavior.