fix: resolve 36 findings from the Jul 24 upstream review by bobleer · Pull Request #1757 · GCWing/BitFun

bobleer · 2026-07-25T07:00:52Z

Follow-ups from a review of the 16 commits merged to main on Jul 24 (#1727,
#1728, #1736, #1739, #1742, #1743, #1746, #1748, #1750, #1751, #1752, #1754).

44 findings were raised; each was re-checked against the tree at 21c0382d4
before anything was changed. 41 confirmed, 3 were not defects as described.
This PR fixes 36 of them. Six areas are touched, so there is one commit per area
— reviewing commit-by-commit will be much easier than reading the combined diff.
Contributing guidance asks for focused PRs; these are independent fixes bundled
only because they came out of one review pass, and I am happy to split them.

The ones worth reading first

Zombie SSH sessions (fix(remote-ssh)) — dropping a russh Handle closes
the socket without telling the peer, so the server keeps the session until its
own timeout. With ProxyJump that is N+1 abandoned sessions per disconnect, one
per hop, accumulating against MaxSessions. Two of the six leak sites build
sessions that are never registered anywhere, so nothing else could ever have
closed them — the connection dialog's Test button leaked a session per hop, per
click.

Reconnect stampede (fix(remote-ssh)) — worse than the review described. A
successful reconnect installs a new alive Arc, so the flag captured before
taking the lock stays false forever and every queued waiter reconnects again.
The lock exists to prevent exactly that. Fixed by moving the locks out of the
map entry that gets replaced, and re-reading liveness under the lock.

China detection overriding a definite answer (fix(relay)) — a resolved
country of US/DE/JP fell through to a connectivity probe that reports "CN"
whenever mirrors.aliyun.com is reachable and github.com is not. Aliyun
answers from anywhere; GitHub has outages, rate limits and corporate blocks. One
transient GitHub failure was enough to rewrite a Frankfurt VPS's apt sources,
point Docker at a Chinese registry and route GitHub clones through a third-party
proxy.

Unverified root code execution (fix(relay)) — in CN mode the Docker install
script defaulted to a jsDelivr copy of docker/docker-install@master: a floating
ref, edge-cached for hours, over a third-party CDN, executed as root with no
checksum or signature. Now requires a pinned digest or byte-identical content
from a second independent origin, failing closed.

apt over https before ca-certificates exists (fix(relay)) — mirror init
runs before anything is installed, and minimal cloud images use http://
sources precisely because there is no CA bundle. The rewrite broke the very
apt-get update that would have installed one, with a TLS error that says
nothing about mirrors.

Unbounded download into memory (fix(release)) — the checksum can only be
verified once the whole body has arrived, so a hostile origin can stream until
the process is OOM-killed with no integrity check ever running.

Mirrored assets were never checksummed (fix(release)) — the sync script
copies an archive and its .sha256 from GitHub without comparing them. Clients
verify an archive against a sidecar from the same host, which proves nothing if
both were copied from a bad download.

The remaining commits cover a elementFromPoint layout thrash on every scroll
event, a one-frame glow flicker, a modal scroll lock that unlocked while another
modal was open, a search loop burning ~22 redundant layout passes, and a
"tool not installed" message shown for tar: link: Not found in archive.

Deliberately not changed

recursion_limit = "256" — measured rather than assumed. At 128 the build
fails on Vec<slab::Entry<h2::…recv::Event>>: Send, ~15 frames of which are
inside h2 itself. Box::pin cannot collapse third-party depth; only erasing a
mid-chain future to Pin<Box<dyn Future + Send>> would, at the cost of an
allocation and dynamic dispatch on the dialog-turn path. Rationale recorded in
lib.rs.
The light-theme glow filter chain — saturate/brightness are the look
style(web): brighten light theme mouse glow #1751 shipped; the accent mix resolves to a blue that the filters push to a
bright cyan no color-mix of the token reproduces. Removing them is a design
call, not a bug fix. Only the provably invisible hue-rotate(4deg) was
dropped.
docker exec -it <c> <shell> -lc <shell> is not a nested double shell.
Shells exec the final simple command of -c, so one process lands on the
PTY, with the login profile sourced — which a bare docker exec -it … sh
would skip. Documented at the call site instead.

Compatibility

No persisted format changed: no config schema, no manifest field, no Tauri
command signature, no on-disk state layout. Mixed-version interop was checked
for the update lock (old fs::write vs. new create_new — compatible via the
shared staleness rule) and for the background-install marker (an older child
ignores the new env var).

Six hazards in the first cut of these fixes were caught and corrected before
this PR, most of them worth naming because they are the kind that only bite in
production:

Tagging a failed hop with anyhow's .context() replaces the error's
Display, which would have turned every user-visible SSH failure into
"connection stage 'target'". Replaced with a transparent wrapper; a test pins
{}, {:#} and the tag together.
An age-based sweep of pid files would delete the pid file of a supervised
command still running after an hour — that file is how cancellation finds the
process. Now liveness-based (kill -0).
Automatic reconnect on config drift would have killed an in-flight exec or PTY
on a still-healthy session. Stale handles are only closed when nothing else
holds the Arc; an explicit disconnect() keeps no such guard.
disconnect_all() now does network sends, so it is bounded at 1s per handle —
it runs on app shutdown, and the session loop may be wedged on an unwritable
socket.
On re-deploy the new apt path would have overwritten the saved upstream
sources.list with already-mirrored content, leaving restore_apt nothing
real to restore.
Checksum verification globbing *.sha256 also matched Desktop sidecars, whose
format that script does not own; scoped to the manifest's own asset list.

Path rejection was narrowed to .. only — . is not traversal and ./name has
always worked — and restart_managed_daemon checks both dirs::config_dir()
and ~/.config so a unit from an older install is still found.

Testing — fully tested (automated); UI changes not visually verified

cargo check and clippy clean on every touched crate. 386 integrations lib
tests, 217 desktop tests, 10 CLI self-update tests, 2048 web-ui tests, 37 i18n
contract tests, tsc --noEmit and eslint all pass. New tests cover the stage
tag's message preservation, the pid sweep sparing a live process, the narrowed
tool-missing detection, remote path traversal, and the prevent-sleep rollback
and ConfigReloaded paths.

tests/file_watch_contracts.rs has 3 failures on my machine that are
pre-existing — confirmed identical against a clean stash of main.

Two changes alter appearance and I have not verified them visually, so they
deserve a look: dropping hue-rotate(4deg) from the light-theme glow, and
removing !important from the interactive-surface transition-property. The
latter means components declaring their own transition now fully own it, which
is the contract the zero-specificity :where() default should have had, but it
can change hover animation on buttons that were relying on the override.

AI-assisted (Claude Code); every finding was re-verified against the code before
being acted on, and the three that did not hold up are called out above rather
than "fixed".

…lization Dropping a russh `Handle` closes the socket without telling the peer, so the server keeps the session until its own timeout. With ProxyJump that is N+1 abandoned sessions per disconnect, one per hop, and they accumulate against the server's MaxSessions. Send SSH_MSG_DISCONNECT before dropping, target first then hops in reverse so each goodbye still has a channel to travel over. Applied to disconnect, disconnect_all, reconnect, re-connect under a live id, container listing and test_connection — the last two build sessions that are never registered anywhere, so nothing else could ever have closed them. Bounded at 1s per handle: the call queues onto a session loop that may be wedged on an unwritable socket, and this runs on app shutdown. Automatic reconnect also skips handles another task still holds, so config drift cannot kill an exec or PTY that is mid-flight. Reconnect serialization was broken in a way the review understated: a successful reconnect installs a *new* `alive` Arc, so the flag captured before taking the lock stays false forever and every queued waiter reconnects again — the exact stampede the lock exists to prevent. Move the locks out of ActiveConnection into a map keyed by connection id (the entry they lived in is replaced wholesale) and re-read liveness from the map under the lock. Also: - Attribute a failed connection to the exact hop with a transparent StagedError wrapper rather than matching message text, which blames the wrong host when two hops share a user@host:port label. Deliberately not an anyhow context: that would replace the message the user sees. - Refuse keyboard-interactive multi-hop without a per-hop IdentityFile instead of replaying one host's OTP at the next, and warn — in the log and in the connection dialog — when a bastion will receive the target's password. - Clean up orphaned scratch files left by an abruptly closed channel. Pid files are swept by liveness (`kill -0`), never age: a supervised command may run for hours and its pid file is how cancellation finds it. - Reach one more generation with `pkill -P` when `setsid` is unavailable and the process-group signal cannot work. - Say what to do in the local-container sshd validation error.

…ript Region detection let a heuristic override a definitive answer. A resolved country of US/DE/JP fell through to the connectivity probe, which reports "CN" whenever mirrors.aliyun.com is reachable and github.com is not — and mirrors.aliyun.com answers from anywhere while GitHub has outages, rate limits and corporate blocks. One transient GitHub failure was enough to rewrite a Frankfurt VPS's apt sources to Aliyun, point Docker at docker.1ms.run and route GitHub clones through a third-party proxy. Treat any resolved country code as authoritative; run the heuristics only when there is no answer at all. In CN mode the Docker install script defaulted to a jsDelivr copy of docker/docker-install@master — a floating ref, edge-cached for hours, fetched over a third-party CDN and then executed as root with no verification at all. Require either a pinned BITFUN_DOCKER_INSTALL_SHA256 or byte-identical content from a second independent origin. Fails closed, with BITFUN_DOCKER_INSTALL_ALLOW_UNVERIFIED=1 as the explicit opt-out; the Aliyun docker-ce path is GPG-verified and already covers Debian/Ubuntu/RHEL. The host apt list was written with a hardcoded `https://`. Minimal cloud images use `http://` sources precisely because ca-certificates is not installed, and mirror init runs before anything is installed — so the rewrite broke the very apt-get update that would have installed the CA bundle, with a TLS error that says nothing about mirrors. Pick the scheme from what the host can actually verify; package signatures are what protect apt either way. Also: - Probe apt-get update after rewriting sources and restore the originals on failure, so a bad mirror is reported as a bad mirror instead of surfacing three steps later as an unattributed error. - Give unsupported distros the same BitFun-owned-list-plus-rename treatment, so restore_apt can actually roll them back; an in-place sed left the host pinned with only a timestamped backup to find by hand. Never overwrite an existing .bitfun-disabled — on a re-run that would replace the real upstream sources with already-mirrored content. - Clean up docker.list/docker.asc on every failure path, refuse an empty GPG key, and stop returning success after a failed install — which made the caller skip its fallback and fail later at `systemctl enable --now docker`. - Prefer HTTPS for country lookups; the answer decides which mirrors a host gets, so a tamperable plain-HTTP body should not be the first choice. - Skip re-detection when the mode is already resolved in this shell. - Drop the redundant [registries.*] cargo table and git-fetch-with-cli, which the relay builder has no `git` to honour.

…ored assets The archive is read fully into memory with no ceiling, and the checksum can only be verified once the whole body has arrived — so a hostile or misconfigured origin can stream until the process is OOM-killed without any integrity check ever running. Cap it. Automatic updates install from a detached child whose output goes to /dev/null, so a repeatedly failing update was indistinguishable from one that never ran. The child now records why it gave up and the next interactive launch reports it. InstallLock used check-then-write, leaving a window in which two `bitfun update` processes both see no lock — inside the very protection against interleaved binary swaps. Use create_new, clearing only a stale lock. The openbitfun sync script mirrored release assets without ever running the checksum it mirrors alongside them. Verifying an archive against a sidecar from the same host proves nothing if both were copied from a bad download, so a corrupt archive would be published and every downstream install would fail against a checksum that matched. Check before publishing the manifest, and delete a mismatched archive so the next run re-fetches instead of skipping it. Scoped to the manifest's own assets: the directory also holds Desktop packages whose sidecar format this script does not own. Also: - Report progress during long downloads instead of showing nothing for minutes. - Attach rollback failures to the error. `let _ = fs::rename(...)` hid the one case that matters — the difference between "the update did not apply" and "there is no working bitfun on this machine any more". - Resolve the systemd user unit through dirs::config_dir() so XDG_CONFIG_HOME is honoured, still checking ~/.config so a unit from an older install is found.

…flicker handleViewportChange is registered for scroll with capture: true, so it fires for every scroll event from every scrollable descendant — well over 60 a second on a trackpad — and each call synchronously ran document.elementFromPoint, which forces a layout. The rAF was throttled; the layout-forcing work was not. Flag the work in the handler and do the hit test once, in the frame callback. Crossing between adjacent surfaces hard-hid the overlay: pointerout/pointermove deactivated synchronously, then the next frame showed the new surface, leaving one frame with no glow on every card-to-card move. Let the frame decide instead — it hides only when no surface is found. findSurface read getComputedStyle twice per element: once for the ignore/resize guard over the whole path, once in the surface search. One pass, one style read, identical semantics. Also: - Emit after initialize() reads the stored preference, so a subscriber that attached first is not left showing the default — a toggle reading "on" while the glow is off. - Drop hue-rotate(4deg) from the light-theme filter chain. Four degrees is invisible once saturate has clipped the blue channel, so it was pure cost on an element repainted every pointer frame. saturate/brightness stay: they are what the light-theme look is, and no color-mix of the accent token reproduces them.

…e-layout Modals stack — a dialog opening a confirmation on top of itself is routine — and each cleared body overflow on its own, so closing the inner one re-enabled background scroll while the outer was still open. The 180ms exit delay widened the window. Ref-count the lock across instances. The centralized interactive-surface rule re-asserted its property list with !important, which forced all eight properties onto components that had chosen a longer duration for one of them, and made `transition: none` unattainable — so an element being dragged animated its way to the pointer. Drop it and let the zero-specificity :where() default lose to component-owned transitions, which is the contract it should have had; [data-motion='none'] remains the opt-out. Search navigation re-armed a rAF on every successful frame once any collapsed content had been expanded, because the flag that recorded the expansion was never reset. The loop only stopped at the 24-attempt ceiling, so ~22 frames re-ran a full DOM tree-walk, re-highlight, ancestor getComputedStyle scan and scroll recompute to reproduce a position that had already converged. Re-arm only while the resolved position is still moving. Also build the folded search text from the same per-character lowercasing the offsets are built from. Whole-string toLowerCase is not guaranteed to agree in length, and any divergence leaves holes in the offset array — the lookup then returns undefined and a real match is silently dropped.

…lifecycle remote_tool_missing matched any message containing the tool name and "not found", so `tar: link: Not found in archive` was reported as "Remote server does not have 'tar' installed" — telling the user to install something they already have while hiding the real cause. Restrict to shell-level phrasing. get_prevent_sleep_enabled started a thread and took an OS inhibitor as a side effect of a read, blocking on D-Bus to do it. spawn_config_listener already owns runtime state at startup and on every config update, so the getter can just read. Its worker round-trip is now bounded: keepawake is a blocking D-Bus call to logind with no timeout of its own, and a hung session manager would hold the mutex forever, wedging every later toggle including the one that turns the feature off. Also: - Reject `..` in the parent of a remote archive path, not only in the file name; the parent is cd-ed into before the command runs. - Give `git rev-parse` on a date filter a timeout, and log when a value resolves to the current second — approxidate never rejects input, so a typo silently becomes a filter that matches nothing. - Extract the prevent-sleep rollback so it can be tested, and cover the failed save, failed rollback and ConfigReloaded paths. - Record why recursion_limit = "256" stays. Measured rather than assumed: at 128 the build fails on Vec<slab::Entry<h2::...recv::Event>>: Send, ~15 frames of which are inside h2 itself. Box::pin cannot collapse third-party depth; only erasing a mid-chain future to Pin<Box<dyn Future + Send>> would, at the cost of an allocation and dynamic dispatch on the dialog-turn path.

Pre-existing flake, unrelated to the rest of this PR; it happened to lose the coin toss on this run's ubuntu job. The three tests in this file run as threads of one process. `legacy_reports_a_missing_primary_without_recursing` copies the launcher to a tempdir and execs it; while `fs::copy` has the destination open for writing, a sibling thread forking for its own `Command` gives the child an inherited write descriptor, and `execve` of that path then fails with ETXTBSY until the child clears it. The descriptor is CLOEXEC, so the window is tiny — invisible until a loaded runner widens it. The writer is already closed by the time `fs::copy` returns and the inherited copy is out of the caller's hands, so retrying is the fix available here.

bobleer · 2026-07-25T07:12:51Z

CI's CLI Tests (ubuntu-latest) job failed on the first run. Investigated — it is not caused by the changes in this PR, but I pushed a fix since it was blocking:

thread 'legacy_reports_a_missing_primary_without_recursing' panicked at src/apps/cli/tests/compat_entrypoint.rs:52:
run isolated deprecated launcher: Os { code: 26, kind: ExecutableFileBusy, message: "Text file busy" }

compat_entrypoint.rs is untouched by this PR — the only bitfun-cli file it changes is self_update.rs, and neither path reaches the other: the legacy launcher just execs its sibling and never calls into self_update, and --version exits before the auto-update hook. The full -p bitfun-cli suite (340 unit + all integration tests) passes locally.

The cause is a Linux-only race between the file's three tests, which run as threads of one process. legacy_reports_a_missing_primary_without_recursing copies the launcher into a tempdir and execs it; while fs::copy holds the destination open for writing, a sibling thread forking for its own Command hands the child an inherited write descriptor, and execve of that path returns ETXTBSY until the child clears it. The descriptor is CLOEXEC, so the window is tiny — which is why it is green almost always and lost the coin toss here.

test(cli): retry the compat launcher exec past a Linux ETXTBSY race retries the exec. The writer is already closed by the time fs::copy returns and the inherited copy is out of the caller's hands, so retrying is the fix available at this layer. Happy to split it into its own PR if you would rather keep this one to the review findings.

Separately, and left alone as out of scope: cargo clippy -p bitfun-cli --all-targets fails on services-core/src/json_store.rs:335 (create without truncate). Reproduced on main with this branch stashed, and CI does not run clippy, so it is neither new nor blocking.

bobleer added 7 commits July 24, 2026 23:58

bobleer merged commit e3169cc into GCWing:main Jul 25, 2026
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve 36 findings from the Jul 24 upstream review#1757

fix: resolve 36 findings from the Jul 24 upstream review#1757
bobleer merged 7 commits into
GCWing:mainfrom
bobleer:fix/upstream-review-followups-2026-07-24

bobleer commented Jul 25, 2026

Uh oh!

bobleer commented Jul 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bobleer commented Jul 25, 2026

The ones worth reading first

Deliberately not changed

Compatibility

Testing — fully tested (automated); UI changes not visually verified

Uh oh!

bobleer commented Jul 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant