Repository navigation
fix(deploy): Copy stacks without rsync when the job image has none - #899
Merged
Merged
Conversation
A spin-up on a Forgejo runner got through the whole server build and stopped in pre-bootstrap: ✗ stack-sync: failed — transport (FileNotFoundError) `rsync` is not in the runner's job image. Measured against `node:22-bookworm`: rsync missing, while ssh, scp, tar 1.34 and gzip are there. Python raises FileNotFoundError for a missing executable, which is an OSError, so it landed in the phase's transport arm and read like a network fault. `_remote.push_directory` now picks the transport at call time: rsync when it is on PATH — every GitHub runner, so that path is untouched — and tar over ssh when it is not. Installing rsync instead was the other option and was measured rather than argued. It works: the rsync and libpopt packages extracted without root, plus LD_LIBRARY_PATH, give a working `rsync 3.2.7` in that image. It was rejected because the pin rots — Debian removes old point releases from its pool, and the URL tried first (`deb12u3`) already 404s while only `deb12u6` remains. A pinned-and-verified installer whose URL disappears on Debian's schedule is a worse failure than the one being fixed, because it arrives months later and looks unrelated. The fallback writes an archive and sends it, rather than piping `tar | ssh`: a pipeline hides the writer's status behind the reader's, which is the shape of #883. Each half is checked on its own. The archive keeps mkstemp's 0600 — one caller pushes Infisical secret values — and is removed in a `finally`, including when ssh fails. Extraction uses --no-same-owner: the numeric uid of whatever account the CI job runs as means nothing on the server, and every destination here is root-owned service configuration. `SSHClient.rsync_to` had its own copy of the rsync invocation, identical and identically broken. It now delegates, so there is one transport in the codebase rather than two. `_transport_detail` no longer calls a missing program a transport failure. It reports `executable not found: rsync (not installed in this job image)`, which is the sentence that would have saved the hour this cost. Guards in tests/unit/test_push_directory.py, 12 of them, and the tar path is exercised for real: a fake `ssh` on PATH runs the remote command locally, so the assertions are about which files arrived and with which modes. Dotfiles (every stack carries a rendered .env), 0600 modes, a created destination, delete-clears-stale, no-delete-keeps-generated-files, a failing ssh raising CalledProcessError with stderr, and a refused `host`-without-path. Six mutations, each caught: no fallback, no mkdir, no delete, dotfiles excluded, no host:path validation, and no cleanup of the archive. The last one was NOT caught at first, and the test was wrong rather than the code: it set TMPDIR, but tempfile resolves and caches its directory on first use, so the assertion inspected a directory nothing had written to. It now patches `tempfile.tempdir` and fails when the cleanup goes. The existing rsync tests in test_remote.py and test_ssh.py pinned the argv but not the transport, so they would have passed on GitHub and failed on a job image without rsync — the very machine this is for. Each now takes an explicit `rsync_installed` fixture. Measured end to end: the full unit suite passes with a PATH that has no rsync in it at all (3641 passed), which is the environment a Conductor fork's CI actually runs in. Closes #897
From the local CodeRabbit round on c04a6ba, and valid. The remote half of the tar transport opened with `set -euo pipefail`. `ssh host "<cmd>"` runs the command in the remote account's login shell, which is not guaranteed to be bash: dash answers `-o pipefail` with "Illegal option", which is exactly how a container job failed in #886. It works on Ubuntu, where root's shell is bash, and would have broken on the first server whose shell is not — for nothing, because the script has no pipeline for pipefail to protect. Three commands, no pipes. Now `set -eu`, which is POSIX. Two guards, each mutation-tested with a recording fake ssh that captures the command string: putting `pipefail` back fails test_the_remote_script_runs_in_a_posix_shell, and dropping the `shlex.quote` on the destination fails test_the_remote_path_is_quoted — the path reaches a remote shell as part of a command string, so the quoting is load-bearing rather than tidy. Refs #897
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @stefanko-ch, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 3 hours and 9 minutes by commenting @sourcery-ai review. Upgrade to get a review now.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: stefanko-ch/Nexus-Stack/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesRemote transport fallback
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Caller
participant push_directory
participant rsync
participant ssh
participant RemotePath
Caller->>push_directory: upload directory
alt rsync is available
push_directory->>rsync: copy directory contents
else rsync is unavailable
push_directory->>ssh: run remote extraction script
ssh->>RemotePath: create, clear, and extract contents
end
push_directory-->>Caller: return CompletedProcess
Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk remains; failed uploads may leave temporary staging incomplete but do not clear deployed service directories. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideDirectory pushes now use the existing rsync path when available and transparently fall back to a POSIX-compatible tar archive transferred over SSH when rsync is absent, while preserving deletion, permissions, error propagation, and cleanup semantics. Missing executables receive actionable diagnostics, duplicated SSH push logic is removed, and the transport behavior is covered with real fallback-path tests plus host-independent existing tests. Sequence diagram for directory push transport selectionsequenceDiagram
participant Caller
participant Remote as _remote.push_directory
participant PATH
participant Rsync as _rsync_push
participant Tar as _tar_push
participant SSH as ssh
Caller->>Remote: push_directory(local, remote, delete)
Remote->>PATH: shutil.which(rsync)
alt rsync available
Remote->>Rsync: _rsync_push(local, remote, delete)
Rsync->>Rsync: subprocess.run(rsync)
Rsync-->>Caller: CompletedProcess
else rsync absent
Remote->>Tar: _tar_push(local, remote, delete)
Tar->>Tar: tempfile.mkstemp()
Tar->>Tar: subprocess.run(tar -C local -cf archive .)
Tar->>SSH: subprocess.run(ssh host script, archive)
SSH-->>Tar: CompletedProcess
Tar->>Tar: archive.unlink()
Tar-->>Caller: CompletedProcess
end
Flow diagram for missing executable diagnosticsflowchart TD
A[Executable invocation fails] --> B{FileNotFoundError with filename?}
B -- Yes --> C[executable not found: program not installed in this job image]
B -- No --> D[Existing transport error detail]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
stefanko-ch
pushed a commit
that referenced
this pull request
Sep 19, 2026
🤖 I have created a release *beep* *boop* --- ## [0.82.3](v0.82.2...v0.82.3) (2026-09-19) ### 🐛 Bug Fixes * **ci:** Keep the Forgejo Access service token across a teardown ([#895](#895)) ([6784086](6784086)) * **deploy:** Copy stacks without rsync when the job image has none ([#899](#899)) ([d083531](d083531)) * **service_env:** reject empty DOMAIN in render_all_env_files ([#863](#863)) ([#867](#867)) ([5c6e93d](5c6e93d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). ## Summary by Sourcery Release version 0.82.3 with fixes for CI token persistence, deployment stack copying, and invalid domain configuration. Bug Fixes: - Preserve the Forgejo Access service token during teardown. - Support copying deployment stacks when rsync is unavailable in the job image. - Reject empty DOMAIN values when rendering service environment files. Chores: - Release version 0.82.3 and update the changelog. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Closes #897.
What broke
A spin-up on a Forgejo runner got through the whole server build and stopped in pre-bootstrap:
rsyncis not in the runner's job image. Measured against the image the runner declares:ssh,scp,tar1.34,gzip,openssl,ssh-keygenrsyncPython raises
FileNotFoundErrorfor a missing executable. It is anOSError, so it landed in the phase's transport arm and was reported as a transport failure — which reads like a network fault and is a missing binary. GitHub'subuntu-latestships rsync, so this cannot reproduce upstream.Approach
_remote.push_directorypicks the transport at call time: rsync when it is on PATH — every GitHub runner, so that path is byte-for-byte unchanged — and tar over ssh when it is not.Why not install rsync
That option was measured rather than argued, and it works: the
rsyncandlibpopt0packages extract without root, and withLD_LIBRARY_PATHthe image has a workingrsync 3.2.7.It was rejected because the pin rots. The project installs pinned tools with pinned checksums (
install-tool.sh), and Debian removes old point releases from its pool — the first URL tried,rsync_3.2.7-1+deb12u3, already 404s; onlydeb12u6remains. An installer whose artifact disappears on Debian's schedule produces a deploy failure months later that looks unrelated to the commit that caused it. That is a worse failure than the one being fixed.snapshot.debian.orgkeeps every version forever and would fix the rot, at the cost of putting a slow, rate-limited archive in the path of every deploy. One problem traded for another.How the fallback is built
Four deliberate choices:
tar | ssh. A pipeline hides the writer's exit status behind the reader's, which is the shape of fix(ci): install-opentofu.sh fails at random with exit 141 —tofu version | headunder pipefail #883. Here each half is a separate call whose status is checked on its own.set -eu, notset -euo pipefail.ssh host "<cmd>"runs in the remote account's login shell, which is not guaranteed to be bash; dash answers-o pipefailwith "Illegal option", exactly as a container job did in fix(ci): Install every lifecycle tool the job image may lack #886. There is no pipeline here for pipefail to protect.--no-same-owner. The numeric uid of whatever account the CI job runs as means nothing on the server, and every destination here is root-owned service configuration. Modes are preserved, which is what the 0600 Infisical payload needs.mkstemp's 0600 and is removed in afinally, including when ssh fails — one caller pushes secret values through it.SSHClient.rsync_tocarried its own copy of the rsync invocation, identical and identically broken. It now delegates, so the codebase has one transport rather than two.The diagnostic
_transport_detailno longer calls a missing program a transport failure:A missing binary and a dropped connection need different fixes, and the old wording sent the reader to the network.
Guards
tests/unit/test_push_directory.py, 14 tests. The tar path runs for real: a fakesshon PATH executes the remote command locally, so the assertions are about which files arrived and with which modes — the argument list was never the thing that broke.Covered: dotfiles (every stack carries a rendered
.env), 0600 modes, a destination created when missing, contents-not-the-directory,delete=Trueclearing stale files including hidden ones, no-delete keeping server-generated files, a failing ssh raisingCalledProcessErrorwithstderr(whichstack_syncprints per service), a refusedhost-without-path, no archive left behind, a POSIX remote script, and a quoted remote path.Eight mutations, each caught by the intended test: no fallback, no
mkdir -p, nofind -delete, dotfiles excluded from the archive, nohost:pathvalidation, no cleanup of the archive,pipefailback in the remote script, and an unquoted destination.One of them was not caught at first, and the test was wrong rather than the code.
test_no_archive_is_left_behindsetTMPDIR, buttempfileresolves and caches its directory on first use, so the assertion inspected a directory nothing had ever written to and passed for the wrong reason. It now patchestempfile.tempdir.Two existing test files would have passed here and failed on the target machine.
test_remote.pyandtest_ssh.pypinned the rsync argv but not the transport, so on a job image without rsync they would fail — on the very machine the fallback exists for. Each now takes an explicitrsync_installedfixture.Measured end to end
The full unit suite passes with a PATH that contains no rsync at all — 3643 passed — which is the environment a Conductor fork's CI actually runs in.
pytest tests/unit: 3643 passed. Pre-commit (ruff, mypy strict, shellcheck, actionlint): all hooks pass.Not verified yet
A real spin-up on the Forgejo runner. The transport is exercised against a local fake ssh, not against the server, and the phase that failed is the one after the last verified step. The next Conductor lifecycle is what settles it.
Side finding, filed separately
A static sweep of every executable the deploy package spawns turned up one more that the job image lacks:
htpasswd, whose docstring states that "every CI runner that runs this code has apache2-utils installed". It bites only when Filestash is enabled, and the fix is a dependency decision rather than a bug fix — filed as #898.Local CodeRabbit round
Reviewed
c04a6ba8: 1 finding, valid, fixed inf8135050.set -euo pipefailin the remote script makes it bash-only for no benefit. Nowset -eu, with a guard that fails ifpipefailreturns.An earlier attempt at the round could not run (rate limit, all included reviews used); this one did.
f8135050itself was not reviewed locally; this review covers it.Summary by Sourcery
Support stack deployment from runner images without rsync while preserving existing synchronization behavior and improving failure diagnostics.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests