fix: pre-release audit fixes (safety, perf, quality, tests, CI) - #17
Merged
Conversation
Fan-out-verified fixes from the 9-dimension pre-release audit (0 release-
blocking; 15 should-fix + clear nice-to-haves applied; design-heavy items
deferred with rationale). Verified green: cargo fmt/clippy -D warnings,
93 unit + 4 integration tests, cross-compile windows-gnullvm/macOS, MSRV 1.88.
Safety / PID-identity ("never signal a recycled pid"):
- proc: Unix shutdown_process_group cmdline-gates before signalling (CR-1);
macOS cmdline_contains parses KERN_PROCARGS2 argv boundaries instead of
substring-searching the env blob (CR-2); sysctl alloc capped (EH-2);
Windows liveness via WaitForSingleObject, not STILL_ACTIVE 259 (WIN-1);
unknown needle refuses instead of degrading (WIN-5).
- worker: re-validate dir (resolve_dir + is_sensitive_dir) before binding,
so direct `ft run-worker --dir /etc` is refused (SEC-1).
- cmd: is_sensitive_dir fails closed when $HOME unresolvable (SEC-4); prune
uses cmdline identity for foreground staleness (CR-3); kill no longer
unwraps tunnel_pid (EH-4).
Robustness / errors:
- registry: backup promoted only if it parses+validates (CR-6); re-seal
0600 on load (SEC-3); compact save encoding (PERF-4).
- cmd: registry-cleanup failures logged, not silently dropped (EH-1);
`ft logs --follow` carries partial bytes across reads (UTF-8/line split).
- name: bound unique_name scan (EH-3).
Performance / async:
- server: blocking fs (canonicalize/stat) moved off the async path (PERF-1).
- worker: coalesce log-tee writes; await readers after abort (ASYNC-5/6,
PERF-2); server.log filter trace->info to bound growth (PERF-3).
- cmd: start poll stats registry mtime, re-parses only on change (PERF-5).
- output: borrow public_url instead of cloning (quality-5).
Quality:
- Remove dead one-variant ServiceKind enum + `kind` field (backward-
compatible: serde ignores the unknown field on old registry.json files).
Tests (+23 unit, +4 integration):
- worker runtime (TC-1), prune classify (TC-2), Windows proc FFI (TC-3),
integration black-box via the real ft binary (TC-4), EntryGuard drop
(TC-5), cloudflared url extraction (TC-7), registry save/load round-trip
(TC-8).
Release / CI:
- Make cargo-deny/cargo-audit + installer-lint gates blocking (REL-2/6);
require the release tag input (REL-1); document `ft prune` + `--yes`
(REL-3); fix stale release.yml comment (REL-4).
Deferred (design-level, need a decision — listed in the PR):
WIN-3 WIN-4 WIN-2 ASYNC-1/2/3 SEC-2 quality-3 PERF-6.
The WIN-5 unknown-needle debug_assert! in proc.rs's #[cfg(windows)] module
used a trailing `needle` arg. 1.88 clippy's uninlined_format_args flags it
(stable demoted that lint to allow), and the line only compiles on Windows,
so it surfaced only on the windows-latest/1.88 leg. Inline as {needle:?}.
Verified: cargo +1.88 clippy --target x86_64-pc-windows-gnu -D warnings clean.
Confirmatory merge-readiness audit of PR #17 (8 dims, adversarially verified) found one real merge-blocker + over-called/test-quality items: - release.yml (MERGE-BLOCKER): all 6 actions/checkout pins were a corrupted SHA (1c1d1d...; the valid v7.0.0 SHA is 1c1d...). Every release.yml job — incl. the new REL-2 supply-chain gate — would have failed at checkout on the first release. Reverted to the valid SHA that ci.yml/main already use. (ci.yml was unaffected.) - proc.rs: the WIN-5 unknown-needle arm used debug_assert!(false, ...), which panics under `cargo test` (debug_assertions on) and a unit test drives exactly that path. Removed the unconditional assert; the refusal (return false) is retained, so the WIN-5 fix stands. - worker.rs: await_entry_returns_false_on_timeout_when_absent used a past deadline, so the poll-loop body never executed (vacuous). Now seeds a non-matching id + a one-interval future deadline so the loop loads, searches, sleeps, then times out. - prune.rs: two tests had names contradicting their assertions; renamed to match what they verify. The audit's "TC-3 merge-blocker" was over-called: CI is empirically green (debug_assertions are on, so the would-panic test cannot have run), making it a latent smell, not a CI failure — fixed above regardless. Verified: fmt; clippy -D warnings (stable + 1.88/x86_64-pc-windows-gnu); 93 unit + 4 integration tests pass.
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.
Applies the fixes from a 9-dimension, adversarially-verified pre-release audit (61 agents, 0 release-blocking). 38 fixes applied; 11 deferred (design-level — see below). Branch verified green before opening:
cargo fmt,cargo clippy --all-targets --all-features -D warnings, 93 unit + 4 integration tests, cross-compilex86_64-pc-windows-gnullvm/aarch64-apple-darwin/x86_64-apple-darwin, andcargo +1.88.Applied
Safety — the "never signal a recycled PID" invariant
proc: Unixshutdown_process_groupnow cmdline-gates before any signal, mirroring Windows (CR-1); macOScmdline_containsparsesKERN_PROCARGS2argv boundaries instead of substring-searching the env blob (CR-2); sysctl alloc capped (EH-2); Windows liveness viaWaitForSingleObjectnotSTILL_ACTIVE259 (WIN-1); unknown needle refuses (WIN-5).worker: re-validates dir sensitivity before binding → directft run-worker --dir /etcis refused (SEC-1).cmd:is_sensitive_dirfails closed when$HOMEis unresolvable (SEC-4); prune uses cmdline identity for foreground staleness (CR-3); kill no longerunwrap()stunnel_pid(EH-4).Robustness/errors — registry backup validated before promotion (CR-6), re-seal 0600 on load (SEC-3), compact save (PERF-4); cleanup failures logged not swallowed (EH-1);
ft logs --followcarries partial bytes across reads (UTF-8/line-split fix);unique_namescan bounded (EH-3).Perf/async — blocking fs moved off the axum request path (PERF-1); coalesced log-tee writes + await readers after abort (ASYNC-5/6, PERF-2);
server.logfilter trace→info to bound growth (PERF-3); start poll stats registry mtime (PERF-5); borrowpublic_url(quality-5).Quality — removed the dead one-variant
ServiceKindenum + field (backward-compatible: serde ignores the unknown field on existingregistry.json).Tests (+23 unit, +4 integration) — worker runtime (TC-1), prune
classify(TC-2), Windows proc FFI (TC-3), black-box integration via the realftbinary (TC-4),EntryGuarddrop-on-panic (TC-5), cloudflared URL extraction (TC-7), registry save→load round-trip (TC-8).Release/CI —
cargo-deny/cargo-audit+ installer-lint gates now blocking (REL-2/6); releasetaginput required (REL-1); README documentsft prune+--yes(REL-3); stale release.yml comment fixed (REL-4).Deferred (need a design decision — intentionally not forced)
confinecanonicalize andServeDiropen — full fix needs replacing ServeDir with anO_NOFOLLOWhandler (design).kill.rs+worker.rs(Job-creation-failure path) — needs both, tracked separately.current_exe) and the unreachablectrl_c()arm — semantic redesign./proc/sysctl/registry-fsync on async threads — needs a coordinatedspawn_blockingpass across the call sites; partial fixes were deemed inconsistent.How it was done
A find→verify audit workflow produced the findings; a second workflow applied them across 8 file-disjoint groups in parallel (no edit collisions), then a manual integration pass resolved 3 breakages (a wrong
WAIT_TIMEOUTimport module, a clippy redundant-closure, a test seed inconsistency) + the cross-fileServiceKindremoval.