Fix .env fallback semantics for binary services#64
Merged
munezaclovis merged 6 commits intomainfrom Apr 20, 2026
Merged
Conversation
Two bugs: (1) pv service:stop (no args) applies .env fallbacks to binary services it didn't actually stop; (2) pv service:remove/destroy for binary services don't apply fallbacks even though the service is permanently gone. Three surgical edits to fix both.
Three issues from the review agent: 1. Binary remove/destroy paths were missing reg.UnbindService — added. 2. Docker destroy doesn't apply fallbacks (spec incorrectly claimed it did) — added Edit 4 to fix the pre-existing Docker bug. 3. Tests didn't pin the actual command-path changes — added targeted tests for the stop-all skip loop and the mail fallback wiring.
Two tasks executing the revised design spec: 1. Three new tests (mail fallback, stop-all skip, unbind) + fix stop.go to skip binary services in the no-args fallback loop. 2. Add fallback + unbind to binary remove/destroy paths; fix Docker destroy to apply fallbacks before unbinding (pre-existing bug). Two commits to land on the branch.
The no-args stop command skips binary services in the container-stop
loop (they're managed by the daemon) but then applied .env fallbacks
to ALL registered services, including binaries that were never stopped.
This rewrote e.g. MAIL_MAILER=smtp → log in linked projects' .env
while mail was still supervised and listening.
Fix: skip inst.Kind == "binary" entries in the fallback loop.
Also adds three new tests:
- mail fallback integration (pins FallbackMapping("mail") wiring)
- stop-all loop simulating the correct skip behavior
- unbind clears ProjectServices.Mail
Binary service:remove and service:destroy now apply .env fallbacks (e.g. MAIL_MAILER=smtp → log) and call reg.UnbindService to clear ProjectServices.Mail/S3 flags on linked projects, matching the existing behavior of their Docker counterparts. Also fixes a pre-existing Docker bug: service:destroy did not call applyFallbacksToLinkedProjects before unbinding, unlike service:remove which does both. Docker destroy now applies fallbacks too.
PR review flagged that TestStopAllFallbackLoop_SkipsBinaryServices re-implemented the loop logic from stop.go inside the test, so removing the production guard would not break any test. Fix: extract the fallback loop into applyStopAllFallbacks(reg) in hooks.go, call it from stop.go, and test the extracted function directly. Now deleting the binary-skip guard causes the test to fail.
munezaclovis
force-pushed
the
fix/binary-service-fallbacks
branch
from
c4249a7 to
3c14ce6
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
1 participant
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.
Summary
pv service:stop(no args) no longer applies.envfallbacks to binary services it didn't actually stop. Previously rewrote e.g.MAIL_MAILER=smtp → logwhile mail was still supervised and listening.service:removeandservice:destroynow apply.envfallbacks and callreg.UnbindServiceto clearProjectServices.Mail/S3flags on linked projects, matching Dockerservice:removebehavior.service:destroyunbinds projects but doesn't apply fallbacks first (unlikeservice:removewhich does both).Bugs fixed
pv service:stop(no args)pv service:remove mailpv service:destroy mailpv service:destroy mysql(Docker)Test plan
go build ./.../go vet ./.../go test ./...— 30 packages passTestApplyFallbacksToLinkedProjects_Mail,TestStopAllFallbackLoop_SkipsBinaryServices,TestUnbindService_ClearsMailBindingpv service:stopwith a linked project using mail — verify.envkeepsMAIL_MAILER=smtppv service:destroy mailwith a linked project — verify.envgetsMAIL_MAILER=log