Round-4 audit: 35 contained fixes - #5
Merged
Merged
Conversation
…tes a successor's lock ReleaseLockFenced's non-fence-error fallback used to run the detached UNCONDITIONAL release: a guarded rm -rf that timed out but completed, or a transport failure after a takeover, deleted whatever lock held the path — including a successor's. The fallback is now one shell-level conditional that removes the lock only when its info file still names the releasing owner (or the lock is already gone); a lock naming anyone else is left strictly alone. MockExecutor gains GuardTransportFailures (guarded commands failing with a transport error) and models the conditional + mv -fT shapes.
…t cache misses
T15: ListContainers requests Labels as a JSON object via a custom --format
({{json .Labels}}) instead of {{json .}}'s comma-joined display string;
splitting that display could not distinguish separators from commas inside
values, so an unrelated label like 'text,teploy.version=bad' forged a
reserved teploy label in the parsed map and steered rollback/prune at the
wrong containers. ParseContainers decodes the object form (legacy display
string still accepted for old-format producers only).
T17 (contained half): ImageExists distinguishes a proven 'no such image'
reply from every other inspect failure — the old &&/|| framing turned a
daemon outage or permission error into a convincing 'missing', steering
callers into pulls and stale-local fallbacks against a broken connection.
The resolve-ID-warns-and-falls-back deploy posture (A52) stays deliberate;
narrowing A17's remainder.
…bindings, env off the argv
T12: InspectRecreate now captures docker's EFFECTIVE top-level mount
inventory. Anonymous volumes created by Dockerfile VOLUME directives appear
in neither HostConfig.Binds nor HostConfig.Mounts, so every recreate
attached a fresh empty volume while the original data lingered on disk;
they are now preserved BY NAME, merged by destination against the requested
spec, and an effective mount the CLI cannot represent fails the inspect
instead of silently dropping storage. --mount values are CSV-encoded.
T13: the recreation renderer brackets IPv6 binds via net.JoinHostPort and
validates ports/protocol/bind IP ('::1:49152:80' concatenation is gone),
with the ephemeral host-port form preserved.
T19 (recreation half, the TCL-12 registered follow-up): resolved env
(secrets included) is staged to a private 0600 on-target file and passed
via --env-file instead of -e arguments, keeping it out of the host process
list and command-bearing diagnostics; values an env file cannot represent
fail closed. The docker-exec AWS/MySQL half stays deferred (A33).
…d worker verification, inventory-retried fallback cleanup T06: rollback's route phase (upstream-port inspections, SetRoute, SetLoadBalancerHealth) used to return directly on failure after the target restarted — leaving the uncommitted target running and, under fixed ports, Caddy pointed at the stopped current container. Route failures now unwind through the same cleanup as start/health failures (stop started, restore displaced). T07: restoreDisplacedAndStarted never set its restored flag on SUCCESS, so an all-predecessors-restarted recovery still reported 'no container is serving'. Successful restarts now count as restored, and partial cleanup failures are joined into the returned error instead of only printing. T21: worker verification treats a persistently unreadable inspect as a deploy FAILURE after bounded retries (reversing A23's degrade-to-warning posture — unknown is not readiness; a deploy can no longer commit while unable to prove any worker exists). T63: the name-derived cleanup fallback retries the container inventory first (it derives worker names from the NEW config, so a worker removed this deploy was invisible to it and kept consuming jobs); every fallback stop/remove failure is reported instead of silently dropped. The predecessor selection is extracted and shared with the snapshot path.
…attempt retention, asset cleanup on the live tree, record identity T10: the asset-bridge seed selector is mtime-ordered and skips attempts whose assets directory does not exist — the lexicographically-greatest pick could select an env-only or build-only attempt and silently seed nothing, dropping the cached asset files older releases accumulated. T11: PruneAttempts bounds the attempts retained per KEPT hash to the two newest (records name the newest attempt of their hash; the second covers a lockless same-hash build racing the committed deploy), so repeated same-version or failed attempts no longer retain build trees, env files, and certificates forever. asset_keep_days cleanup now runs on the LIVE attempt-scoped tree (the one the container mounts) in addition to the legacy shared path — it had been a no-op for every deploy since F08. T56: releasemeta.Read validates the record's embedded App/Hash against the requested key — a copied, migrated, or corrupted-but-valid record can no longer drive rollback/recreate effects at a different release's spec.
… status observed, local parent-dir fsync T45: every atomic publication on the supported GNU/Linux target renames with mv -fT (no target-directory semantics) — remote Upload's staged script, UploadAtomic, and secret Set. A plain 'mv -f -- tmp dest' with the destination a symlink TO A DIRECTORY succeeded by nesting the staged file inside it while the expected destination stayed unchanged, a false-success write. T41: secret List runs a bare find with its exit status observed and sorts in Go — the old 'find | sort' pipeline had no pipefail, so a failed find reported 'no secrets' and deploys proceeded without the app's secrets. Listed names are validated against the key grammar at the source. T46 (local half): LocalExecutor.Upload fsyncs the containing directory after the rename, completing the local crash-durability contract (file synced, rename persisted). The remote-shell fsync contract stays deferred with the durability family.
…overy armed before the baseline copy, post-stop baseline T51: .teployignore now EXTENDS the always-protected defaults (node_modules, .git, .env, .env.*) instead of replacing them — one custom pattern used to ship .env and .git to the build host where a broad COPY bakes them into the image; an unreadable ignore file is an error rather than a silent defaults-only transfer. T37: the redis restore script defines restore_original and captures the baseline AFTER the stop, with the capture's failure explicitly compensated (restart + abort) — under set -e the old copy exited the script immediately and left Redis stopped with no recovery attempt. T38: the baseline is captured against the STOPPED container (docker cp), distinguishing a proven 'no such file' (nothing to preserve) from every other failure — the old pre-stop existence flag missed the final RDB a graceful shutdown writes when none existed before. The regression tests drive the generated script under a real bash with a stub docker.
…validated release ids The pin read-modify-write now holds the same fenced app lock every deploy, prune, and rollback holds: two unlocked pin edits could both succeed while one pin silently disappeared, and a pin could race a prune that had already read an older protection set — a successful pin was not a retention guarantee. Pin values are validated against the release-id grammar at the command boundary (they later key prune sets and meta paths); ValidateHash is exported from releasemeta for that boundary.
… fields validated to what they render T23: publish entries are parsed against a documented, narrow grammar ([ip:]host:container[/proto], single numeric ports, bracketed IPv6) at BOTH boundaries — teploy.yml load and the shared deploy validator direct construction goes through — and duplicate host bindings (including wildcard-vs-specific) are rejected before any accessory, volume, route, or workload mutation; malformed entries used to reach docker after the fixed-port predecessor had already been stopped. T53: basic_auth values must be COMPLETE structural bcrypt hashes (the old prefix-only check broke the reload at runtime instead of the config load), the forward_auth verify URI must be request-path-shaped when configured, copy_headers entries must be HTTP tokens, and the upstream URL rejects control characters — closing the TCL-39 renderer-input follow-up.
…rsisted in the Caddyfile, honest autodeploy hygiene T26: the webhook route now lives IN THE CADDYFILE, rendered inside the app's managed site block from a persisted per-app descriptor (/deployments/<app>/.webhook-route) — the runtime admin-API injection existed only in Caddy's memory and was erased by the next ordinary deploy's regeneration+reload, including by webhook-triggered deploys themselves. Every managed render (deploy, rollback, maintenance on/off) re-applies the fragment under the same lock + adapt gate + reload/verify transaction; a corrupt descriptor aborts the edit rather than silently dropping the route. T27: the route honors the CONFIGURED listener port (9876 was hardcoded) and matches every configured domain (the comma list was inserted as ONE host value); SetupCaddyRoute takes the port, DefaultPort is the single constant. T29: autodeploy Schedule/Unschedule read the crontab with its exit status checked (only the canonical no-crontab message starts from empty) and the 'crontab -r' fallback is gone — a failed read used to wipe every unrelated cron job, and a failed replacement used to remove the ENTIRE crontab. T30: Remove aggregates every step failure (stop/disable/unit delete/reload, unschedule, route removal) into an 'incomplete' error instead of printing 'removed'; Status reports transport failures as errors, never 'inactive'. T31: the resident path resolves relative TLS cert/key paths against the fetched checkout (the systemd unit has no WorkingDirectory). T32: the webhook secret is stored and verified VERBATIM — setup rejects whitespace-wrapped secrets and serve refuses (with the reason) instead of silently trimming the HMAC key.
…, detached recovery waves, maintenance serialized with deployment
T57: a failed load-balancer update after a fully successful wave is now a
nonzero exit ('backends deployed but load-balancer activation failed') —
the old shape printed a warning and returned nil, so a green CLI exit
proved nothing about reachability while backends served new versions on
new ports behind old targets.
T58 (contained half): both fleet rollback waves (partial failure and
canary gate) run on bounded detached recovery contexts — the deploy
context is signal-cancelled exactly when the operator interrupts, and
recovery that skips itself on the dead context is how a Ctrl-C strands
half a fleet on the new version. The generation-identity half of T58
(compensating only the recorded predecessor) stays deferred with the T04
family.
T62 (contained half): maintenance on/off takes the app's fenced deploy
lock, the --app path verifies the AUTHORITATIVE server ingress mode instead
of assuming caddy, the stash is read inside the Caddyfile mutation
transaction, and it is deleted only when this transaction actually restored
one — a concurrent maintenance-on's fresh stash can no longer be clobbered
by a stale read or deleted by a racing off. The versioned-desired-state
redesign stays deferred.
…ter context, proxy-free probes, least-privilege workflows T48 (narrowing A47): update extraction is bounded and single-binary — declared member sizes are checked before any byte is read, reads are limited, non-regular and duplicate matching entries are refused, and the entry count is capped. The old bare io.ReadAll let a small compressed member expand until memory exhaustion before any checksum ran. T49 (cancellation half): the updater derives its context from the Cobra command, so Ctrl-C actually cancels the check/download/verify sequence; the update-selection policy (downgrades, prereleases) stays deferred (A48). T20 (contained half): health probes pass curl --noproxy '*' — an inherited HTTP_PROXY made host-local readiness ask a proxy about a loopback address. The explicit HTTP/TCP mode redesign stays deferred (A22). T61: CI and the release workflow are read-only by default; contents:write is granted only to the publishing job. (GoReleaser version pinning remains the A49 owner item — pins are not invented here.)
…ding deferrals with evidence folded in
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.
Fenced-release guards, webhook persistence, crontab safety, publication atomicity. 35 of 63 findings fixed with regression tests; 28 standing deferrals folded. Race suite green.