fix: retire deactivated-profile containers on apply/upgrade (#795) - #806
Closed
VijitSingh97 wants to merge 2 commits into
Closed
fix: retire deactivated-profile containers on apply/upgrade (#795)#806VijitSingh97 wants to merge 2 commits into
VijitSingh97 wants to merge 2 commits into
Conversation
A local→remote node switch dropped the node's profile token from COMPOSE_PROFILES, but nothing stopped the running container: compose does not treat a profile-disabled service as an orphan (the --remove-orphans comment claimed otherwise), and while the container survives, newer compose treats the service as enabled again — so the TARI_MEM_LIMIT diff that rides every tari switch recreated the very node the switch had just removed, leaving a full minotari node running offline against a remote-mode config. apply and upgrade now stop and remove every container whose compose profile the committed .env switched off — monerod (local_node), tari (local_tari), and the two payout-confirmation wallets — before the recreate `up`, keyed off the committed .env (not this run's diff) so an interrupted apply's marker retry converges too. Bind-mounted chain data is untouched. Stack tier covers the transition end-to-end: the docker stub models per-service container state as flag files, and the new cases drive the tari and monerod local→remote switches through the real apply and assert the container is absent afterwards (plus removal-before-up ordering, and a quiet steady-state re-apply). Closes #795 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ponytail-review pass: same logic, one variable per arm instead of two. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Superseded by #804, which landed the same root cause (profile-disabled services are still defined, so --remove-orphans never drops them — independently confirmed here) with the removal hooked into compose_up_checked, a strictly broader placement than this PR's apply/upgrade call sites, plus equivalent stack-tier coverage of both node switches, the payout wallets, and removal-before-up ordering. Nothing left to salvage. 🤖 Generated with Claude Code |
Collaborator
Author
|
Post-close review notes, for the record:
|
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.
Closes #795
Root cause
applynever stops a deactivated-profile container itself — it delegated the whole cleanup todocker compose up -d --remove-orphans, whose comment claimed "--remove-orphans drops monerod when a local→remote switch deactivates the local_node profile". That claim doesn't hold: a profile-disabled service is still defined, so compose does not count its container as an orphan — and while the container survives, newer compose treats the service as enabled again. Net effect on the bench repro: the switch droppedlocal_tarifromCOMPOSE_PROFILES, then theTARI_MEM_LIMITdiff that rides every tari switch (auto-calc → the remote-mode0mplaceholder) made the samecompose uprecreate the node the switch had just turned off.So the issue's hypothesis was close but the mechanism is one level down: there is no pithead-built recreate list at all — the "recreate set" is compose's own resolved-config diff, and the bug was that nothing removed the container before compose ran, so compose still considered the service live.
monerod was equally affected (
local_node, same delegation to--remove-orphans— #103's e2e only ever proved fresh-remote), and so were the two payout-confirmation wallets (wallet-rpc/payout_confirm,tari-wallet/tari_payout_confirm) when a view key is cleared.upgradere-renders.envfromconfig.jsontoo, so a mode switch staged in config could land there with the same hole.Fix
One shared guard,
remove_deactivated_profile_containers: after the.envcommit and before the recreateup, stop and remove every container whose compose profile is not active in the committed.env(the four profile-gated services). Keyed off the committed.envrather than this run's diff, so an interrupted apply's marker retry (#125 path) converges too. Called from bothapplyandupgrade. Bind-mounted chain data is untouched, exactly as the apply preview always promised.Coverage
Stack tier (the lowest tier that honestly proves the transition): the shared docker stub now models per-service container state as flag files (
compose --profile X ps -aq svc/rm -sf svc), and new black-box cases drive the realapplythrough:up;Testing
tests/stack/run.sh: 1693 passed, 0 failedtests/stack/test_compose.sh: greenmake lint: green🤖 Generated with Claude Code