Skip to content

test(cloud-lab): #497 crash-window harness — SIGKILL between mint-signed and wallet-save - #569

Closed
Amperstrand wants to merge 1 commit into
mainfrom
test/crash-injection-lane
Closed

Amperstrand wants to merge 1 commit into
mainfrom
test/crash-injection-lane

Conversation

@Amperstrand

Copy link
Copy Markdown
Collaborator

Implements the last #497 deliverable at the TollGate layer: the SIGKILL-at-boundary harness — literally the issue's acceptance criterion ("kill between POST-swap and SaveProofs…; on restart the wallet completes the swap and the proofs appear").

How the deterministic kill works

killerproxy.py sits in the compose network between the tollgate and the mint (the tollgate's config points its accepted mint at the killer service). All traffic passes through except /v1/swap responses after the mint has signed the first one:

  1. the first watched 200 writes a marker (mint HAS signed) and begins swallow mode;
  2. every subsequent watched response is also swallowed — a one-shot swallow lets the wallet library's connection-retry ladder (up to 5 attempts) complete the operation behind the kill and defeats the injection; the retry ladder is exactly what has to be held open;
  3. the driver SIGKILLs the tollgate the moment the marker appears, then touches a release file — from then on the killer passes through, which is precisely what the boot-time intent replay needs.

The scenario

run-crash-injection.sh (same isolation conventions as lab.sh): isolated project/subnet/container names, no host ports, all mints + killer up before the tollgate boots (a boot-order mistake starts it degraded — lesson #6 in the RUNBOOK), fund and pay through the killer so the token's mint URL matches the tollgate's accepted spelling, kill, restart, then two assertions: the recovery log line, and a fresh payment returning 200.

Verified live (against the intent train: gonuts #31/#33 + boot-resume wiring)

marker: /v1/swap                       ← mint signed
killed at 14:57:13                     ← SIGKILL mid-wait
wallet: pending op 87eb08e9… recovered 50 sats from http://killer:8085
post-recovery payment: 200

Against pre-intent binaries the recovery assertion fails by design — the harness exists to prove the window costs value without the machinery. The lane needs the gonuts intent PRs merged + the TMBG boot-resume wiring to pass on main; until then it is runnable manually against the stacked branch (how it was verified).

Also in this PR

  • README test-suite row; changelog entry.
  • make go-battery: 16 modules green (shell/python only, no Go changes).

@Amperstrand

Copy link
Copy Markdown
Collaborator Author

Technical review (evidence pass)

Ran the harness for real, twice, on a shared docker host (unique compose project, dedicated subnet, full teardown verified): it does what the body claims, the kill point is deterministic, and a full run costs ~5 minutes. Two small blockers below (both one-liners); the design itself is sound and is the right shape for #497's TollGate-layer acceptance criterion.

What I ran

COMPOSE_PROJECT_NAME=cl-crash-sjrw-pr569  ./run-crash-injection.sh   # 4m46s, exit 1
COMPOSE_PROJECT_NAME=cl-crash-sjrw-pr569b KEEP=1 ./run-crash-injection.sh  # same shape, kept for forensics

Both runs: mints+killer up → tollgate last (accepted mints correctly rewritten to http://killer:8085) → funding + payment → marker → SIGKILL → restart → FAIL: no recovery after restart. That FAIL is the expected outcome on this branch — I confirmed gonuts #31/#33 (the swap-intent PRs) are still open, so the image built here is pre-intent; the body says exactly this. I did not rebuild against the stacked intent train, so the passing path is taken on the author's transcript, not re-proven here; everything up to and including the assertion is reproduced.

The killer's request log (docker timestamps, run 2) is the determinism proof:

20:53:12.5–14.6  POST /v1/mint/quote/bolt11, POST /v1/mint/bolt11   ← client funding, passed through
20:53:16.362     POST /v1/swap                                        ← tollgate swap #1 → mint 200 → marker + swallow
20:53:17.027     POST /v1/swap                                        ← wallet retry ladder fired → swallowed again
20:53:17.18      SIGKILL                                              ← 0.8 s after swap #1, mid-window

The 17.02 retry is precisely the case a one-shot swallow would have let complete behind the kill — the persistent-swallow design at killerproxy.py:72-93 is not just theory, it carried the window. Marker semantics are sound: the marker is written exactly when the mint's 200 reaches the proxy, and the wallet provably cannot complete between marker and kill.

One worry I checked and ruled out: could the client's funding swap arm the marker early? Empirically no — the killer logs every request before forwarding, and the client's cdk-cli send never POSTs /v1/swap (this cdk-cli prefers offline sends; send --help documents --tolerance as "amount willing to overpay to avoid a swap"). Funding traffic in the log is quote/mint only, and the single swap POST follows the payment launch. If a future unpinned cdk-cli ever swaps on send, funding fails loudly (create_cashu_token errors when its swap is swallowed), not silently at the wrong boundary — acceptable, though pinning the cdk-cli version in Dockerfile.client (pre-existing) would remove the question entirely.

Runtime/leak-safety of the script itself: every wait is bounded (mint 40×1 s, marker 60×1 s, recovery 40×3 s, HTTP timeouts on all client calls) — no hang path I can find. Isolation claims verified live: no host ports (docker port empty on mint/upstream), dedicated 172.31.77.0/24 subnet, per-project container names, and the config-rewrite trick correctly keeps the token's mint URL spelled identically to the accepted mint.

Blocker 1 — cleanup leaks the killer container and the network

run-crash-injection.sh:39-47 runs down -v via the COMPOSE array, which is built at L33-37 from docker-compose.yml only — but the killer service exists only in $RUN_DIR/override.yml. Compose therefore treats the killer as an orphan and leaves it running, and the still-attached container pins the lab network. Both happened on my first run: after cleanup() I found cl-crash-sjrw-pr569-killer still Up and cl-crash-sjrw-pr569_tollgate-lab still present. On a shared host (or a long-lived CI runner) that is one leaked running container + network per run, which contradicts the lane's own isolation contract. Fix is one line — include the override in the down:

"${COMPOSE[@]}" -f "$RUN_DIR/override.yml" down -v --remove-orphans >/dev/null 2>&1 || true

I verified this exact invocation removes everything (used it to tear down my KEEP=1 run; containers, network, and volumes all gone).

Blocker 2 — changelog entry links the wrong PR

CHANGELOG.md:212 links [#562](…/pull/562); this PR is #569 and the entry describes this harness. Please renumber to #569, and drop the doubled blank line after the entry (L213-214) so it matches the file's one-blank-line style. The entry text itself is accurate, as is the new README row at tests/cloud-lab/README.md:97.

Boundary coverage vs the AGENTS.md list

AGENTS.md enumerates five kill boundaries. This harness covers exactly one of them — and the one #497's acceptance criterion names:

None of that blocks this PR — its stated scope is the last TollGate-layer deliverable, and it delivers that.

Smaller notes

  • "same isolation conventions as lab.sh" (L19-21) and "lesson Build/Deployment improvements #6 in the RUNBOOK" reference files that don't exist on main — the conventions as they exist today live in run-keyset-rotation.sh / run-rejection-safety.sh (the env knobs do match, so the substance holds). If the run-scoped runner lands separately, fine; otherwise point at what's mergeable.
  • The background payment's pay.log captures only compose's container-creation output; the actual pay: <status> line goes to the detached container's logs, which down -v destroys. When a run fails mid-scenario, the payment's fate is unrecoverable. Cheap fix if you touch the file again: --name the run container and docker logs it in the FAIL paths, or drop -d and let wait collect it after the kill.
  • The restart at L198 emits an orphan-container warning for the still-running payment container — cosmetic, disappears with the fix from the pay.log note.
  • One fragility worth a comment line in killerproxy.py: the swallow gate requires code == 200, so a mint that answers the wallet's re-POST with a non-200 NUT-19 error mid-window would pass through. cdk-mintd re-signs identical replays (gonuts Duplicate tollgate SSIDs #31's tests assert "exactly two identical signings"), so it holds for this lab; a nutshell-style mint might diverge. The release file already bounds the exposure to the ≤1 s marker poll.
  • Not wiring this into CI is correct today (it fails on main by design); the follow-up should be explicit: wire it into the cloud-lab automation the moment gonuts Duplicate tollgate SSIDs #31/Feature/test joel frontend #33 and the TMBG boot-resume wiring merge, otherwise the harness silently rots as a manual-only lane.

Hygiene: single well-structured commit, no attribution footers, base is 8 commits behind main but all four files are new/append-only so no conflicts are expected; no new dependencies (killer reuses Dockerfile.client); nothing here touches the portal-facing surface or the built package.

Disposition

Request-changes — both items are one-liners: (1) make cleanup() tear down with the override file so the killer container and network don't leak per run (live-proven leak, fix verified), and (2) fix the changelog PR link #562 → #569 (+ stray blank line). Once those land this is a clear merge: the kill point is genuinely deterministic, the retry-ladder defeat is real rather than theoretical, and the run fits comfortably in minutes.

@Amperstrand

Copy link
Copy Markdown
Collaborator Author

Technical review (evidence pass) — PR head 5f2201e4e9377cd694aa6627e0ce2d85f13430bc (base 21 commits behind main).

The branch head does not contain this PR. The body describes the #497 crash-injection harness — killerproxy.py, run-crash-injection.sh, a compose wiring for the killer service, a README suite row, a changelog entry, and a live verification transcript. The actual head contains none of it. I checked out 5f2201e4 and searched the full tree: no killerproxy.py, no run-crash-injection.sh, no crash-injection lane in tests/cloud-lab/ (the suite table at tests/cloud-lab/README.md ends at test_two_router_autopay.py), and no harness entry in the changelog. The "Verified live" transcript and the README/changelog claims cannot be reproduced from this head — whatever branch produced them, it is not the one this PR points at.

What the head does contain is a single commit, 5f2201e4 "docs(changelog): drop the orphaned second [Unreleased] section". That is a real cleanup of a real defect — the merge-base 2796d96c does carry an orphaned second [Unreleased] block after the link-reference list (CHANGELOG.md at merge-base) — but main already fixed it in #577, commit 1377a876, with an identical subject and identical effect (main's changelog has exactly one [Unreleased] header today). So even the one change present is a duplicate of merged work; merging this PR as-is would be contentless.

This is not an isolated accident, and I want to flag the pattern because it has now eaten four PR heads. PR #549 (feat/wallet-recover) and PR #557 (feat/cloud-lab-runner) currently point at the identical head SHA a15be70ff3b414b0139d12f884e9c3c06ad494db — the wallet-recover series has overwritten the cloud-lab-runner branch wholesale. PR #530 (rootfs-validation-tier) has the same symptom: its tip commits are the wallet-recover series (fix(tollwallet): bound checkstate against wedged mints, docs(changelog): link the wallet recover entry…). And now #569's head is a stray happy-path-series commit that belongs to the #544/#554/#577 line of work. Whatever process pushed these branches — scripted push, shared local checkout, worktree mixup — it destroyed the reviewed content of all four PRs. The harness described in this body evidently existed (the transcript is specific and plausible, including the deliberate multi-swallow design to defeat the wallet's 5-attempt retry ladder, which is exactly the right instinct for this window), but it is not reachable from any current PR head.

Since the diff carries no code, the usual criteria are vacuous for what's actually here — no new dependencies, no security surface, nothing to test. But against the described change, one thing deserves to be said before a re-push, because it's the part a maintainer will press on: the killer-proxy design is the correct shape for the #497 acceptance criterion. Killing after the mint's first signed swap response (not after the request) is the only placement that hits the true point-of-no-return; funding and paying through the killer so the token's mint URL matches the tollgate's accepted spelling pre-empts exactly the alias-canonicalization trap that made #375/#480 confusing; and gating the recovery assertion on the intent machinery (rather than weakening it to pass on main) keeps the lane honest — a lane that "passes" without the fix proves nothing. The cloud-lab placement is right too; AGENTS.md says to extend tests/cloud-lab/ lanes rather than invent new harnesses, and this is a crash-window lane next to the existing mint-failure lane.

Blocking items, in order:

  • Re-push the actual harness branch to test/crash-injection-lane and confirm gh pr view 569 --json headRefOid shows the new head before requesting re-review. Before pushing, inspect the local checkout that will do the push — given four contaminated heads, assume the push source is wrong until proven otherwise.
  • Drop the changelog-cleanup commit from the branch; docs(changelog): drop the orphaned second [Unreleased] section #577 already landed it.
  • On the re-push, include the README suite row and changelog entry the body promises (the changelog entry should land under Changed / Internal per the rubric — CI/test lanes have been landing there).

Disposition: request-changes — nothing to review yet; the described change is real, wanted (#497 is the open P0), and the design is sound, but the head must actually carry it. Given the cluster, it may be worth a maintainer-side audit of what else the overwriting push clobbered across #530/#549/#557/#569.

…ned and wallet-save

run-crash-injection.sh places the tollgate's mint traffic behind
killerproxy.py, which swallows every /v1/swap response once the mint
has signed — including the wallet library's connection-retry ladder
(a one-shot swallow lets the retry complete the operation and defeats
the injection; a release file gates the pass-through back on after
the kill, which is exactly what the boot-time replay needs). The
driver SIGKILLs the tollgate at that exact boundary, restarts it, and
requires the intent resume to recover the value; a fresh payment must
then still succeed.

This is #497's acceptance criterion at the TollGate layer: 'Test kills
the process (SIGKILL) between POST-swap and SaveProofs with a mint
that recorded the swap; on restart the wallet completes the swap and
the proofs appear (balance preserved).'

Verified live against the intent train (gonuts #31/#33 + the
boot-resume wiring): marker -> kill mid-wait -> restart -> 'pending op
… recovered 50 sats' -> post-recovery payment 200. Against pre-intent
binaries the recovery assertion fails by design — that is the point.
@Amperstrand

Copy link
Copy Markdown
Collaborator Author

Branch restored and rebased (post-review maintenance). This branch had been force-pushed over with unrelated content (the #577-era changelog commit); the crash-window harness survived only as a dangling commit. Restored from 95fe4575 (killerproxy.py + run-crash-injection.sh + README + CHANGELOG), rebased onto current main (afc86b3f) — applied cleanly. Head is now 252ca33e. The review's request-changes reason ("head must actually carry the change") is thereby addressed; the harness content itself was never reviewed in detail, so a fresh look at the restored diff is warranted.

@Amperstrand

Copy link
Copy Markdown
Collaborator Author

First live run of the crash-injection harness — mechanics PASS, and its verdict on the current tree is (correctly) RED.

Ran run-crash-injection.sh end-to-end on this branch's tree (fresh compose stack: mints + killerproxy + upstream, port-remapped for the shared host):

Which is the finding worth having on record: the #497 exposure is real and unmitigated on today's main — a power loss between the mint's swap acceptance and the wallet's proof save destroys the customer's value today, and this lane is the acceptance harness that any future swap-intent PR (gonuts-tollgate #31/#33-era machinery + the TMBG boot-resume wiring) must turn green. Until then the harness is doing its job by staying red.

Mechanics note for whoever runs it next: it publishes host :2121 (remap on shared hosts) and needs the full killer/mint-rotate stack, ~10 minutes wall clock.

@felixfelix-bot

Copy link
Copy Markdown
Contributor

Conflicts resolved on a rebase of the single commit (252ca33e) onto current main (667d0e47).

Replacement PR: #676 — #676 — base main, head pr/crash-injection-rebased @ c8336dd5, 1 commit, 4 files, +351/−0 (the same shape as this PR).

  • CHANGELOG.md was the only conflict: the crash-window bullet now leads main's #570 (tests map) and #573 (MAC rotation) entries under ## [v0.6.0-rc1] → ### Changed / Internal, kept verbatim — git diff main is its own 11 lines and nothing else; main has no crash-window entry, so no duplicate.
  • killerproxy.py (107) and run-crash-injection.sh (232) are byte-identical to git merge-tree main 252ca33e, as is tests/cloud-lab/README.md (main's newer content plus this PR's one table row).
  • bash -n clean, python3 -m py_compile killerproxy.py clean.

Not run here: the harness needs a tollgate built with the gonuts swap-intent machinery plus the compose lab — and against a pre-intent binary the recovery assertion fails by design, so a run without that binary proves nothing. The commit's live record (marker → kill mid-wait → restart → pending op … recovered 50 sats → post-recovery payment 200) stands; this rebase does not touch that code path.

Two shellcheck findings in the new script — flagging, not silently editing an approved file: SC2081 at :143 ([ "$(… ps --format '{{.Status}}' upstream)" = *"healthy"* ] can never glob-match inside [ ], so that wait always burns its full 30 × 2 s before falling through — [[ … == *healthy* ]] is the fix; and SC2034 at :202 (unused loop variable i). Neither is CI-gating (the workflows do not shellcheck tests/*.sh), but main's other cloud-lab scripts are clean at -S warning, so this would be the first to warn. Say the word and I'll fold the one-liner in here rather than a follow-up.

Merge #676 and this PR closes as superseded, or force-push pr/crash-injection-rebased onto test/crash-injection-lane (recipe in #676's body).

@Amperstrand

Copy link
Copy Markdown
Collaborator Author

Superseded by #676 — the same crash-window harness, already rebased by felix onto current main. Closing this one in favor of #676.

@Amperstrand Amperstrand closed this Oct 6, 2026
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.

3 participants