Skip to content

Deploy: remote_nginx lost REMOTE_OUT to a subshell, 5c refuses to edit a conf without a 2b backup, and the dry-run suite runs in CI - #378

Merged
Apolloccrypt merged 2 commits into
mainfrom
fix/deploy-5c-backup-precondition
Sep 3, 2026
Merged

Deploy: remote_nginx lost REMOTE_OUT to a subshell, 5c refuses to edit a conf without a 2b backup, and the dry-run suite runs in CI#378
Apolloccrypt merged 2 commits into
mainfrom
fix/deploy-5c-backup-precondition

Conversation

@Apolloccrypt

@Apolloccrypt Apolloccrypt commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Vervolg op #376: twee bevindingen van de reviewer, een documentatie-nit, en een
bug die deploy-run 4 stilzette.

0. Run 4 stopte in 2b op een meting die de server wel degelijk had gedaan

Run 4 (TS 20260903-0216, deploy/logs/deploy-3.1-20260903-0216.log) stopte met

STOP .env backup is a real file with content -- the server never printed 'after .env backup bytes'

terwijl de server die regel wel printte (1420 bytes) en beide confs had
opgelost, paramant.conf op 14682 bytes. De meting klopte; het script kon hem
niet zien.

De wrapper uit #376 was een pipeline:

remote_nginx() { { printf '%s\n' "$NGINX_RESOLVE_SNIPPET"; cat; } | remote "$@"; }

Elke stage van een pipeline is een subshell, dus remote() en _remote_run()
zetten REMOTE_OUT en REMOTE_RC in een kind dat daarna afsloot. In de parent
stond nog wat het vorige remote-blok had achtergelaten, dus elke expect na een
remote_nginx-aanroep beoordeelde de verkeerde uitvoer, en een niet-nul
REMOTE_RC uit zo'n blok zou de deploy helemaal niet gestopt hebben.

remote_nginx leest de body nu zelf en geeft remote() zijn stdin via een
redirect (< <(printf ...)), dus remote() draait in de aanroepende shell en
beide variabelen landen waar de asserts kijken.

Op de server heeft run 4 alleen rollback-tags en backups gemaakt. Geen
container hercreeerd, geen conf bewerkt, geen docroot geschreven.

1. 2b en 5c resolven onafhankelijk, dus ze kunnen het oneens zijn

Dat onafhankelijk resolven is met opzet: elke fase vraagt de server wat er op
dat moment in sites-enabled staat. Maar fase 3 en 4 (pull, build, recreate)
zitten ertussen. Wijzigt sites-enabled in dat venster, dan bewerkt 5c een conf
die 2b nooit heeft geback-upt. Elke FATAL in 5c roept restore() aan, en die
kon alleen terugzetten wat gefiled was.

Aangetoond op de oude code, met een replica waar 2b paramant-live.conf filede
en 5c daarna paramant.conf oploste:

FATAL ... Restoring the backed up confs.
available/paramant-public.conf: OK
available/paramant.conf: FAILED

Public hersteld, backend LEFT MODIFIED, terwijl de tekst een volledig herstel
belooft.

Fix. 5c eist per resolved naam [ -f "$NGBK/$name.pre-3.1-$TS" ] vóór de
eerste sed, telt ze in before confs without a backup, en STOPt met een FATAL
die de conf en het gezochte backup-pad noemt. Een run die daar stopt heeft niets
geschreven. De deploy assert lokaal dat de teller nul is. restore() loopt nu
over $RESOLVED_CONFS in plaats van te globben over de backup-map, dus wat het
terugzet is precies wat bewerkt is, en het zegt per conf wat het terugzette.

2. De dry-run-suite draaide nergens

docs/ONBOARDING.md:51 zei het letterlijk: "exists but runs in no workflow".
Een groene PR zei dus niets over het enige script in deze repo dat
productiecontainers hercreëert, en twee rondes nginx-wijzigingen zijn op die
stilte geland.

Toegevoegd als job deploy-dryrun in .github/workflows/test.yml: bash, geen
server, geen secrets, checkout gepind op dezelfde SHA als de andere jobs.
ONBOARDING is op beide regels gecorrigeerd.

Nog niet in de required contexts. Zoals afgesproken: pas toevoegen nadat hij
drie keer groen op main heeft gedraaid. De branch-protection regel je zelf.

Bijvangst: ubuntu-latest heeft shellcheck, dus de shellcheck-poot van de suite
draait daar wel en op een laptop zonder shellcheck niet. Lokaal met shellcheck
0.11.0 erbij: shellcheck -S error schoon op beide bestanden.

3. Documentatie-nit

#376 zei dat fase 6 de opgeloste naam gebruikt. Dat klopt niet. Fase 6 leest
nginx -T, de geladen config, en probet de site over HTTP; geen van beide weet
hoe het bestand op schijf heet. DEPLOY-3.1.md zegt dat nu, met de correctie
erbij benoemd. Het commit-bericht van #376 is gemerged en niet meer te wijzigen.

Bewijs

bash tests/deploy-3.1-dryrun.test.sh: 259 checks groen, was 239. Nieuw:

  • 6i-8 de wrapper zelf: remote_nginx() wordt echt aangeroepen met een
    gestubde _remote_run die REMOTE_OUT zet, na REMOTE_OUT en REMOTE_RC
    eerst met muffe waarden te hebben gevuld. Asserts: de caller ziet de nieuwe
    waarden, de snippet staat voor de body in de stdin die de stub kreeg, en de
    body komt heel aan. De pipe-vorm blijft als negatieve controle in de test, dus
    de check kan niet slagen op een harness die de bug zou doorlaten. Plus een
    grep die elke remote-aanroep rechts van een pipe weigert. De bloktests
    extraheren heredoc-bodies en raken de wrapper niet; daarom miste 252 groene
    checks dit

  • 6i-7 rename tussen 2b en 5c: 5c exit niet-nul, FATAL noemt
    paramant.conf.pre-3.1-..., before confs without a backup = 1, en beide
    confs zijn md5-identiek aan voor de run, dus niets blijft bewerkt achter. Ook:
    de run belooft geen restore meer die hij niet kan waarmaken

  • 6i-7b controle: dezelfde fixture met de backup wél op de opgeloste naam
    loopt schoon door en bewerkt beide confs

  • 6i-7c een FATAL ná de edits zet beide resolved confs terug, byte voor
    byte, en meldt per conf wat het terugzette

  • elke 5c-fixture seedt nu eerst de 2b-backups, wat een echte run ook gedaan
    heeft tegen de tijd dat 5c start

Sabotage, twee keer. De preconditie eruit en restore() terug naar de glob:
7 checks rood, waaronder "5c changed a conf before it stopped" en "5c edited a
conf that has no backup". De wrapper terug naar de pipe-vorm: 3 checks rood,
"REMOTE_OUT did not reach the caller", "REMOTE_RC did not reach the caller (got:
[7])" en de pipe-grep. Beide teruggezet, 259 weer groen.

Verder groen: tests/static-sanity.sh PASS, scripts/check-test-declarations.sh
(114 suites), scripts/check-commit-style.sh, shellcheck -S error.

…p, and the dry-run suite runs in CI

Two things the review of #376 found.

Phase 2b and phase 5c resolve the conf slots independently. That is deliberate:
each asks the server what is in sites-enabled at the moment it runs. It also
means they can disagree, because phases 3 and 4 pull, build and recreate in
between. If sites-enabled is rearranged in that window, 5c resolves to a conf
2b never backed up. Every FATAL in 5c then calls restore(), restore() could
only put back what was filed, and the run ended on "Restoring the backed up
confs" with that conf left edited and nothing to roll it back to. Reproduced:
paramant-public.conf restored, the backend conf LEFT MODIFIED.

5c now requires a backup under this run's TS for every resolved conf, checked
before the first sed, so a run that stops there has written nothing. It reports
"before confs without a backup" and the deploy asserts that it is zero.
restore() walks the resolved names instead of globbing the backup dir, so what
it puts back is exactly what was edited, and it says which conf it restored.

tests/deploy-3.1-dryrun.test.sh ran in no workflow at all, which
docs/ONBOARDING.md said in as many words. So a green pull request said nothing
about the only script in this repo that recreates production containers, and
two rounds of nginx changes landed on that silence. It is now the test.yml job
deploy-dryrun: no server, no secrets, pinned checkout SHA like the other jobs.
Do NOT add it to the required contexts yet; give it three green runs on main
first. ONBOARDING is corrected on both rows.

Documentation fix: #376 said phase 6 uses the resolved conf name. It does not.
Phase 6 reads nginx -T, the config that is actually loaded, and probes the site
over HTTP, so it cannot be affected by what the file on disk is called.
DEPLOY-3.1.md now says that, and names the correction.

The suite goes from 239 to 252 checks. New: the rename-between-2b-and-5c
scenario stops with nothing written and both confs byte for byte unchanged, the
same fixture with the backup in place runs through, and a FATAL after the edits
puts both resolved confs back. Every 5c fixture now seeds the 2b backups first,
which is what a real run has done by the time 5c starts.
…after it judged the previous block

Deploy run 4 (TS 20260903-0216) stopped in phase 2b on

  STOP .env backup is a real file with content -- the server never printed
  'after .env backup bytes'

while the server had printed exactly that line, 1420 bytes, and had resolved
both confs, paramant.conf included at 14682 bytes. The measurement was fine;
the script could not see it.

The wrapper added in #376 was a pipeline:

  remote_nginx() { { printf '%s\n' "$NGINX_RESOLVE_SNIPPET"; cat; } | remote "$@"; }

Every stage of a pipeline is a subshell, so remote() and _remote_run() set
REMOTE_OUT and REMOTE_RC in a child that then exited. In the parent both still
held whatever the previous remote block had left, so every expect after a
remote_nginx call judged the wrong output, and a non-zero REMOTE_RC from such a
block would not have stopped the deploy at all.

remote_nginx now reads the body itself and hands remote() its stdin through a
redirect, so remote() runs in the calling shell and both variables land where
the asserts look.

The block-level tests extract heredoc bodies and never touch the wrapper, which
is why 252 green checks missed this. The new check drives the real
remote_nginx() with _remote_run() stubbed, seeds REMOTE_OUT and REMOTE_RC with
stale values, and asserts the caller sees the new ones, that the resolver still
arrives before the body, and that the body arrives intact. The pipeline
spelling is kept as a negative control, so the check cannot pass on a harness
that would wave the bug through, and a grep refuses any remote call on the
right-hand side of a pipe. 259 checks.

On the server run 4 created rollback tags and backups and nothing else. No
container was recreated, no conf edited, no docroot written.
@Apolloccrypt
Apolloccrypt force-pushed the fix/deploy-5c-backup-precondition branch from fdb1d56 to 435ab4d Compare September 3, 2026 00:25
@Apolloccrypt Apolloccrypt changed the title Deploy: 5c refuses to edit an nginx conf that phase 2b never backed up, and the dry-run suite runs in CI Deploy: remote_nginx lost REMOTE_OUT to a subshell, 5c refuses to edit a conf without a 2b backup, and the dry-run suite runs in CI Sep 3, 2026
@Apolloccrypt
Apolloccrypt merged commit 405c5bd into main Sep 3, 2026
11 checks passed
Apolloccrypt added a commit that referenced this pull request Sep 3, 2026
Since #378 the dry-run suite derives phase 0a's gated list from the
workflow files, and security-posture.yml had a push-to-main trigger, so
it belonged in that list and was not in it.

Adding it would have been the wrong repair. The gate reads the last
completed run of each listed workflow on main, and for this one that is
the nightly external scan. That scan is red on purpose: a missing CAA
record, an unsigned zone, an HSTS header duplicated by a layer above
this repository, and one Rust advisory that carries no severity in any
database and needs a human ruling. Every one of those describes the DNS
zone or the server. None of them says anything about whether main is
deployable, and gating on them is the mistake the runbook already
records about heartbeat.yml.

So the push trigger is gone and the exclusion is written down in all
three places that state it: the comment above REQUIRED_WORKFLOWS, the
--dry-run printout, and DEPLOY-3.1.md.

The selftest loses nothing. It still runs on every pull request, which
is how every change reaches main, and it is the only thing standing
between a green tick and a scanner that cannot report red.

Dry-run suite: 259 passed, 0 failed.
@Apolloccrypt
Apolloccrypt deleted the fix/deploy-5c-backup-precondition branch September 5, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant