[#261] Demarcate output from remote server - #401
Merged
Merged
Conversation
heitor-lassarote
force-pushed
the
heitor-lassarote/#261-demarcate-output
branch
from
September 10, 2026 14:44
08eb0d0 to
1aa4c93
Compare
Problem: It's unclear which output came from the server and which came from the local machine during SSH activation. Solution: Prefix each line coming from the server with a fax machine emoji, using a byte-level reader that inserts the prefix immediately before the first byte of each new line rather than reformatting on newlines, so it also works on long lines with no embedded newline. This required using piped streams rather than inheriting them, so the child's wait is now joined together with the stdout/stderr forwarding tasks, avoiding a deadlock once output exceeds the OS pipe buffer. The reader takes its writer as a parameter, constructed once by the caller, rather than picking between stdout/stderr internally on every write: tokio's stdout()/stderr() explicitly warn that creating a fresh handle per write can reorder output under concurrent load, and taking the writer as a parameter also makes the function unit-testable with an in-memory writer instead of the real, global stdout/stderr. Also adds an e2e NixOS VM test (`prefix-demarcation`) that deploys a custom activation script over real SSH and asserts the compiled `deploy` binary's actual captured output prefixes server-origin lines on both streams while leaving deploy-rs's own local log lines unprefixed.
Problem: revoke()'s SSH session never piped stdout/stderr, so real remote output from a rollback (including diagnostics from the rollback itself failing) either went straight to the terminal unprefixed, or, via wait_with_output(), was captured into buffers that were always empty since nothing was actually piped. revoke()'s failure case promised stderr content in its error message that it could never deliver. Solution: Extract the take-stdout/stderr -> forward_reader -> join(wait) pattern already used by both deploy_profile branches into a read_remote_process_child helper, and use it in revoke() too, now piping its child's stdout/stderr like the activate paths do. This also switches revoke()'s failure case from CommandError::Exit (which needs a fully-buffered Output) to CommandError::ExitStatus, matching the activate paths, since streamed bytes are no longer retained in memory to attach to an error. Add an e2e NixOS VM test (revoke-demarcation) that fails a multi-profile deploy on purpose to trigger a real rollback, and asserts the revoke SSH session's output, including its own rollback-failure diagnostics, is demarcated exactly like a normal activation's.
heitor-lassarote
force-pushed
the
heitor-lassarote/#261-demarcate-output
branch
from
September 10, 2026 16:46
1aa4c93 to
17f5c50
Compare
heitor-lassarote
marked this pull request as ready for review
September 10, 2026 16:47
Sereja313
reviewed
Sep 13, 2026
Member
Yeah, I agree, i think this should be on by default. |
Problem: per-line prefixing changes the actual bytes of every remote line, which can break any script or tool that parses deploy's output expecting it unprefixed. Solution: a new --no-demarcate-output flag lets callers opt out of the prefix entirely. When set, read_remote_process_child skips piping and forward_reader altogether, falling back to plain inherited stdio and wait() exactly as before this feature existed, so scripts relying on the raw output aren't broken. Verified live that both the demarcated and un-demarcated paths still stream output in real time rather than buffering it, matching the fix made in #380/#379 for the same class of regression. Adds an e2e test (no-demarcate-output) covering the new flag, and a unit test asserting forward_reader keeps draining its reader to EOF (rather than abandoning it) when a local write fails, so the remote child's pipe is never left to fill and block.
heitor-lassarote
force-pushed
the
heitor-lassarote/#261-demarcate-output
branch
from
September 14, 2026 16:42
1318a54 to
b94efaa
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
2 participants
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.
Description
Prefixes each line of output forwarded from the remote server's SSH session (activate, wait-for-confirmation, revoke) with 📠, so it's unambiguous whether a given line came from the server or from the local
deployprocess itself.Adds
--no-demarcate-outputto opt out: when set, stdout/stderr are left inherited instead of piped, so a script or tool parsingdeploy's output isn't broken by the new prefix.Related issue(s)
Resolves #261
Notes for reviewers
revoke()previously never piped its SSH child's stdout/stderr at all; this PR fixes that as part of wiring up demarcation for it.forward_readerand new e2e tests (prefix-demarcation,no-demarcate-output,revoke-demarcation).--no-demarcate-output. This changes stdout/stderr byte content unconditionally, including for piped/scripted callers, not just interactive terminal use (unlike--no-progress, which only affects TTY output since progress bars are already suppressed on non-TTY streams). Alternatives considered: default off (opt-in, no risk of breaking existing scripts, but the feature goes mostly unused since it needs discovering), or auto-detect based on whether stdout is a TTY (matches--no-progress's convention, but would also silently disable the prefix in CI log output that a human reads later, which is one of the two scenarios this feature targets). Currently shipping with the opt-out default; open to switching before merge.