fix(wallet): drain safety — canonical mint identity, partial results, fsync'd journal (#375)

[Amperstrand]

Fixes #375.

wallet drain cashu aborted on the first per-mint failure, silently discarding tokens already produced by earlier successful (and irreversible) mints — on wallets whose registry held two URL spellings of one mint this could destroy real funds. Three layers:

• Canonical mint identity — mint URLs canonicalized (scheme/host case, trailing slash) at registration and merged on read; one logical mint can never appear as two phantom-balanced entries.
• Partial results — per-mint failures collected into an explicit result (success:false, partial:true, tokens, per-mint errors) in both plain and JSON output; earlier tokens are never discarded. On the CLI side the partial data is shown before the error so real funds stay visible.
• Crash-safe journal — every produced token is appended to an fsync'd /etc/tollgate/wallet-drain-journal.jsonl (0600, 0700 dir) before the next mint is attempted; a journal failure stops further draining.

Also: --yes/-y for non-interactive drains, and meaningful exit codes (cancellation, full and partial failure, JSON-mode success:false).

Tests: partial-failure retention, journal-before-next-mint ordering, canonicalization/duplicate-registry, drain confirm flow (src/cli/server_drain_test.go, src/cmd/tollgate-cli/drain_*_test.go, src/tollwallet/{mint_canonicalization,duplicate_mint_registry}_test.go).

Verification: go vet + tests green in src/cli, src/tollwallet, src/cmd/tollgate-cli; root gofmt -l/go vet/go build + go test -race -count=1 -tags testenv ./... green. Live regression validated in PRTA #107 (closed).

Note: the CHANGELOG hunk also normalizes [Unreleased] (merges the duplicated Fixed/Added/Changed sections accumulated from the merge-night conflicts, removes one literal #381 duplicate); no entry content changed.

[Amperstrand]

Code: verified green — this is the right fix, well-shaped

Ran the full check battery locally on 45fce6d:

• src/tollwallet, src/cli, src/cmd/tollgate-cli: go test -count=1 -tags testenv ./... all pass; -race -p 1 green on the drain + wallet suites; go vet clean; gofmt clean.
• The six drain tests cover exactly the #375 repro scenarios — partial-failure token retention, duplicate-alias shared balance, journal-before-next-mint ordering, journal-failure-stops-further-drains — plus all-green path.
• All three reported bug layers map to real changes: canonical identity fixed write-side at AddMint (the actual duplicate-creation mechanism — good catch registering the canonical form, not just comparing it), partial results with the historical error-string shape preserved for automation, and the CLI confirm/exit-code semantics (silent no-op → error with --yes guidance; JSON-mode exit 0 → non-zero on success:false).
• Journal design is right: fsync before the next irreversible swap, stop-on-journal-failure direction is the safe one, 0600/0700, bearer-token retention documented.

Release fit: given #375 is live-repro'd fund loss on main (end-to-end, Sep-17 build), this should land before the v0.6.0 stable tag. The changelog entry text itself is accurate.

One blocking nit: the CHANGELOG hunk drops existing [Unreleased] entries

The rewrite keeps this PR's entries and "Reproducible builds", but loses (present on main today, absent after this diff):

• "Release pipeline runs on Nostr CI (no GitHub dependency)" — the #410 ngit-pipeline entry (the largest, ~28 lines),
• "gonuts re-pin to v0.11.2" (#394),
• the .gitignore CLI-binary fix (#411).

Looks like a merge-conflict resolution that took this branch's side. Since [Unreleased] feeds the v0.6.0 release notes directly, please restore those three entries (rebase onto current main and take main's side for the unrelated entries is the easiest route). Happy to re-verify — everything else is 👍 from me.

[Amperstrand]

Correction to my review above (entry-level re-diff, sorry for the noise): the ngit-pipeline / gonuts re-pin / .gitignore entries do survive the rewrite — my keyword-count check was misled by reworded fragments. The only genuinely dropped entry is:

• "Tester guide for the alpha RC." (docs(tester): alpha RC tester guide (install / upgrade / rollback / report) #381 — docs/rc-tester-guide.md, the /etc/apk/world rollback pin, --force-downgrade caveats; feeds the alpha-RC testing programme)

Restore that one entry (Added section) and the changelog is whole. Everything else in the review stands as written: code verified green locally (tests ×12 incl. the #375 scenarios, race, vet, gofmt on 45fce6d), fix shape correct at all three layers, recommend landing before the v0.6.0 stable tag.

[Amperstrand]

Changelog fix pushed (f6a6428): the #381 tester-guide entry is restored; entry-level diff of [Unreleased] vs main is now empty — no other content changes. The PR is MERGEABLE, only awaiting review (self-review doesn't satisfy the gate, per the usual).

Recap for the reviewer: code verified locally on 45fce6d (12 targeted tests covering the #375 scenarios, -race, go vet, gofmt — see review above); recommend landing before the v0.6.0 stable tag given the live-repro'd fund loss on main.

[felixfelix-bot]

Reviewed head: f6a64285393b — merge-base 125f6a06; the merge-base is the live upstream/main tip and git merge-tree is clean (exit 0, one tree OID), so mergeable_state: blocked is neither a conflict nor the dead CI lane: ruleset 18235130 on main requires one approving review and carries no required-status-check rule.

What it does. wallet drain cashu is the pre-wipe command on a gateway, and it was lossy: the old loop returned on the first per-mint failure with Success:false, discarding every token minted that run (src/cli/server.go:291-304), and a declined confirmation exited 0 (main.go:101-104). Now failures accumulate, successful tokens reach the token file before the error prints, each is fsynced to the drain journal before the next mint, mint URLs are canonicalised (#375), and failures exit non-zero. My findings are the edges it leaves open.

1. Mint identity is not total, though the docs claim it is. normalizePath (tollwallet.go:354-362) strips at most one trailing slash; default ports, userinfo, query/fragment and IDN are untouched. So …/Bitcoin/ collapses onto …/Bitcoin (the #375 fix), but …/Bitcoin//, …:443/Bitcoin, user:pass@…/Bitcoin and …?v=1 stay distinct keys for one physical mint: each reports the same keyset balance, so server.go:287 drains it twice, the retry failing on zero. …/Bitcoin%2F also normalises onto the unescaped path, an over-merge in the direction that loses funds. Fix: strings.TrimRight(p, "/") and port/userinfo handling.

2. The read-side alias merge is load-bearing yet untested. Neutralising GetAllMintBalances (tollwallet.go:406-430 → return w.wallet.GetBalanceByMints()) leaves src/tollwallet and src/cli green, while my probe on a wallet holding one mint under two spellings goes from one 50-sat entry to two phantom ones: registerMint normalises first, so the branch credited with repairing existing installs is unexercised. Reuse the probe (tw.wallet.AddMint(aliasWithSlash); one entry / 50 sats). max is right, not a sum: gonuts sums by keyset id.

3. The journal is write-only, unbounded, and its crash claim is overstated. Ordering is genuine (appendDrainJournal in-loop at server.go:323, Write→Sync at drain_journal.go:63-66), but nothing reads it back: grep finds only its definition, call site and test. Drained bearer tokens accumulate forever in /etc/tollgate/wallet-drain-journal.jsonl unless swept by hand (0600/0700 apply only at creation). drain_journal.go:15-22 also says journaling "closes that window" for a crash between swap and response, but gonuts' Send has already put the proofs in pending_proofs, leaving the in-Send window uncovered. Reword: a second copy, not the recovery path (scripts/token-recovery exists).

4. TOLLGATE_TEST_CONFIG_DIR is honoured by shipped code and moves a token sink. drainJournalPath() (drain_journal.go:27-33) and socketPath() (main.go:34-40) branch on it unconditionally in the production binary: set it in a drop-in or wrapper and drained tokens go to an arbitrary directory while the CLI talks to another socket.

5. Test-only: the CLI drain tests are not hermetic w.r.t. TMPDIR. With this host's TMPDIR the package is 29 PASS / 2 FAIL — the partial-failure and JSON-mode failure tests die with bind: invalid argument, as t.TempDir()'s name-derived path exceeds AF_UNIX's 108-byte sun_path; TMPDIR=/tmp gives 31/31.

Acquittals worth recording. Real cliff targeted (drained proofs sit in pending_proofs, so the CLI cannot re-emit them); partial JSON is additive with the old error prefix kept; plain text shows tokens/file before the error; --yes skips exactly one prompt; packaging/ and the feed payload are untouched.

CI: no GitHub CI evidence — gh pr checks 433 returns "no checks reported on the 'fix/375-wallet-drain-safety' branch" (exit 1) because Actions is queued-dead org-wide since 2026-08-27; the ngit lane builds main only, so there is no ngit result here. Locally: src/cli ok 0.130s, src/tollwallet ok 0.233s, tollgate-cli 31/31 under TMPDIR=/tmp. Four mutations: reverting token retention, deferring the journal and reverting normalizeMintURL each went red; neutralising the alias merge did not (finding 2).

RC RECOMMENDATION: LAND BEFORE THE ALPHA RC. It touches src/cli, src/tollwallet and src/cmd/tollgate-cli, so it is in the RC binary either way; the question is only whether testers run it. Their verdicts turn on a multi-mint drain with real sats, exactly where the old code stranded funds in a bucket the CLI cannot re-emit. Landing after the freeze instead costs a re-cut of the feed artifact (.ipk and .apk), a re-install on testers' boards and invalidating results already gathered against the frozen commit, for a fix whose purpose is to stop that drain stranding money. Ship F1/F2 as fix-ups; until F1 lands, testers need one canonical mint spelling.

[Amperstrand]

Ran the affected modules on this branch: go vet + go test -race -count=1 -tags testenv ./... green in src/cli, src/cmd/tollgate-cli, src/tollwallet; root gofmt -l empty. Read the diff in full — the ordering guarantee (journal fsync before the next mint, break-on-journal-failure with the token still in the response), the canonical-identity single-source (registry keys and MintURLMatches both derive from normalizeMintURL, so they cannot disagree), and the backward-compatible error phrasing for string-matching automation all check out. Pairwise merge check vs #432 (the other CHANGELOG-touching PR): no conflicts.

[Amperstrand]

Small branch fixup pushed (eb7050b): the changelog normalization hunk emitted the ## [Unreleased] heading twice in a row (lines 11–12); one copy removed. No entry content changed.

[Amperstrand]

Second small branch fixup (5756d9c): the PR description says the changelog hunk removes one literal #381 duplicate, but both byte-identical copies survived the normalization. The duplicate (the copy after the #411 entry) is removed; no other entry content changed. Net effect of the two fixups: the changelog now matches what the PR description promises.

[felixfelix-bot]

UPDATE at head eb7050bf49 (live, re-derived before posting) — the head moved one commit since my review of this PR (f6a6428539, id 5261470806, artifact round4/post-433.md). I re-derived the delta rather than assuming it: gh api repos/OpenTollGate/tollgate-module-basic-go/compare/f6a64285393bf47c4b0ef02e151e7c10c28b0cfa...eb7050bf49aebbad1bc6621abc13f3d2f29cc306 → ahead_by: 1, behind_by: 0, one file — docs(changelog): drop the duplicated [Unreleased] heading, CHANGELOG.md +0/−1. No code, test, packaging or workflow byte changed, so every finding and the verdict in that review stand unchanged; read them there rather than expecting a second full pass:

• F1 mint identity is not total — normalizePath strips only one trailing slash, so //, :443, userinfo and ?v=1 remain distinct registry keys and %2F over-merges in the loss direction (src/tollwallet/tollwallet.go:354-362);
• F2 the read-side alias merge in GetAllMintBalances is load-bearing and pinned by no test (mutation M3 left the packages green while a probe went 1×50 sat → 2 phantom 50 sat) (tollwallet.go:406-430);
• F3 the journal is write-only, orphanable and unbounded, its 0600/0700 apply only at creation, and its "closes that window" claim is overstated (src/cli/drain_journal.go:15-22,41-70);
• F4 TOLLGATE_TEST_CONFIG_DIR is honoured by shipped production code (moves the token sink and the control socket) (drain_journal.go:27-33, src/cmd/tollgate-cli/main.go:34-40);
• F5 test-only: the new CLI drain tests are not hermetic w.r.t. TMPDIR (29 PASS/2 FAIL with the host TMPDIR, 31/31 with TMPDIR=/tmp — AF_UNIX 108-byte sun_path).

Rebase note (new). This PR's base is 125f6a0645; main is now 52b8481a3b (#435 merged), so it is 1 behind, and the commit above edits CHANGELOG.md — the same file #435's merge touched. Rebase before merging and expect a one-hunk conflict there.

CI: no GitHub CI evidence — gh pr checks 433 --repo OpenTollGate/tollgate-module-basic-go → no checks reported on the 'fix/375-wallet-drain-safety' branch (exit 1); actions/runs?head_sha=eb7050bf49 → total_count: 0 (Actions queued-dead org-wide since 2026-08-27). The ngit lane builds main only → no ngit result for this branch, asserted rather than implied green.

RC RECOMMENDATION: LAND BEFORE THE ALPHA RC (unchanged). It ships in the RC binary either way — src/cli, src/tollwallet and src/cmd/tollgate-cli are all packaged — so the only question is whether testers run it, and their verdicts turn on a multi-mint drain with real sats, which is exactly where the old code strands funds in a bucket the CLI cannot re-emit. Landing after the freeze costs a re-cut of the feed artifact (.ipk and .apk), a re-install on tester boards and invalidating results already gathered against the frozen commit. The one-commit delta is a CHANGELOG.md dedup: zero byte risk, nothing to retest. Ship F1/F2 as fix-ups per the earlier review — until F1 lands, testers need one canonical mint spelling.

[felixfelix-bot]

CORRECTION + UPDATE at head 5756d9ca5a (live, re-derived immediately before posting this). My immediately preceding review of this PR (id 5261613217) was pinned at this same head but its body named eb7050bf49 as the head — that was true when the body was written and false by the time it was posted, because the author pushed again in between. This review supersedes it on the head fact; the findings and the verdict are unchanged from it and from my full review at f6a6428539 (id 5261470806, artifact round4/post-433.md).

The full delta since my reviewed head f6a6428539. gh api repos/OpenTollGate/tollgate-module-basic-go/compare/f6a64285393bf47c4b0ef02e151e7c10c28b0cfa...5756d9ca5aaf571bf0082f62bc6b095cff88b33d → ahead_by: 2, behind_by: 0, one file: CHANGELOG.md +0/−13 across two commits — eb7050bf49 docs(changelog): drop the duplicated [Unreleased] heading and 5756d9ca5a docs(changelog): remove the literal #381 duplicate entry. (Cross-checked the intermediate step separately: eb7050bf49...5756d9ca5a is ahead_by: 1, CHANGELOG.md +0/−12.) No src/, test, packaging/ or workflow byte has changed since the full review, so nothing in it needs re-deriving — read the findings there:

• F1 mint identity is not total — normalizePath strips only one trailing slash, so //, :443, userinfo and ?v=1 stay distinct registry keys and %2F over-merges in the loss direction (src/tollwallet/tollwallet.go:354-362);
• F2 the read-side alias merge in GetAllMintBalances is load-bearing and pinned by no test (mutation M3 left src/tollwallet + src/cli green while a probe went 1×50 sat → 2 phantom 50 sat) (tollwallet.go:406-430);
• F3 the journal is write-only, orphanable and unbounded, 0600/0700 apply only at creation, and its "closes that window" claim is overstated (in-Send window uncovered) (src/cli/drain_journal.go:15-22,41-70);
• F4 TOLLGATE_TEST_CONFIG_DIR is honoured by shipped production code — it moves the token sink and the control socket (drain_journal.go:27-33, src/cmd/tollgate-cli/main.go:34-40);
• F5 test-only: the new CLI drain tests are not hermetic w.r.t. TMPDIR (29 PASS/2 FAIL with the host TMPDIR, 31/31 with TMPDIR=/tmp — AF_UNIX 108-byte sun_path).

Rebase note. The PR base is 125f6a0645; main is now 52b8481a3b (#435 merged), so the branch is 1 behind and all three of its most recent commits edit CHANGELOG.md, the file #435's merge also touched. Rebase before merging and expect a conflict in that section; the dedup commits themselves are the answer there (the duplicate entries they remove are exactly what a stale-base merge would resurrect).

CI: no GitHub CI evidence — gh pr checks 433 --repo OpenTollGate/tollgate-module-basic-go → no checks reported on the 'fix/375-wallet-drain-safety' branch (exit 1); actions/runs?head_sha=5756d9ca5a → total_count: 0 (Actions queued-dead org-wide since 2026-08-27; the newest run anywhere in the repo is a 2026-09-17 "Graph Update" still queued). The ngit lane builds main only → no ngit result for this branch, asserted rather than implied green.

RC RECOMMENDATION: LAND BEFORE THE ALPHA RC (unchanged). It ships in the RC binary either way — src/cli, src/tollwallet and src/cmd/tollgate-cli are packaged — so the only question is whether testers run it, and their verdicts turn on a multi-mint drain with real sats, exactly where the old code strands funds in a bucket the CLI cannot re-emit. Landing after the freeze costs a re-cut of the feed artifact (.ipk and .apk), a re-install on tester boards and invalidating results already gathered against the frozen commit. The two newest commits are CHANGELOG-only: zero byte risk, nothing to retest, and they remove duplicated release notes that would otherwise be published verbatim. Ship F1/F2 as fix-ups per the full review — until F1 lands, testers need one canonical mint spelling.

[Amperstrand]

Review follow-ups landed in 54b043e (+ sibling CHANGELOG hygiene 0b97ce8/04c1fe6 kept on top — head is now 04c1fe6):

Finding	Fix	Evidence
F1 — mint identity not total (multi-slash, default ports, userinfo, query/fragment each split one mint into two drain targets)	normalizeMintURL now collapses any trailing slashes, drops scheme-default ports (443/https, 80/http), and discards userinfo/query/fragment; escaped slashes (%2F) deliberately stay distinct (over-merge loses funds)	New rows in mint_canonicalization_test.go: 6 new equivalent pairs + 2 new distinct pairs (:8443, %2F); TestNormalizeMintURL_CanonicalEquivalence and the MintURLMatches agreement test green
F2 — read-side alias merge load-bearing but untested	TestGetAllMintBalances_MergesLegacyAliasEntries_Issue375: injects a second spelling below the canonicalizing registration layer (as older versions left them), asserts the precondition (underlying wallet really holds 2 entries × 50 sats), then requires ONE merged max-balance entry out	Mutation-verified: neutralizing the merge (GetAllMintBalances → raw GetBalanceByMints()) fails the test; restored → green
F3 — journal's crash claim overstated	drain_journal.go doc now says what the journal is (a second copy of tokens the CLI already holds) and what it is not (recovery for a crash inside the swap — that's wallet.db's pending bucket + scripts/token-recovery)	diff on src/cli/drain_journal.go
F4 — TOLLGATE_TEST_CONFIG_DIR honoured by shipped code	Recorded as #443 (build-tag vs document decision); both branch sites now carry "exists for the test harness only" notes	#443
F5 — CLI drain tests not hermetic w.r.t. TMPDIR (AF_UNIX 108-byte sun_path)	shortTestConfigDir() helper: uses t.TempDir() when <dir>/tollgate.sock fits, falls back to /tmp when it doesn't	Reproduced the original failure (bind: invalid argument under a 100+-byte TMPDIR), then: suite ok under the same deep TMPDIR and under normal TMPDIR

Verification on 54b043e: gofmt -l clean; go vet clean; -race suites green in src/tollwallet (1.59s), src/cli (1.09s), src/cmd/tollgate-cli (0.15s deep-TMPDIR / 4.9s normal) and the root package (18.9s). Root go build ./... clean.

Heads-up for coordination: this branch received two parallel pushes during the follow-up work (a fetch+force-with-lease momentarily clobbered the changelog hygiene pair eb7050b/5756d9c); both are recovered and preserved on top of the fixes above — no work lost, current head 04c1fe6 contains everything.

cc @felixfelix-bot — F1/F2/F3/F5 fixed in-tree, F4 filed as #443; ready for another pass.

[Amperstrand]

Verification pass on the current revision (f6a6428).

• Touched-module battery, all green under -race -count=1 -tags testenv: src/tollwallet (incl. the new mint_canonicalization_test.go + duplicate_mint_registry_test.go), src/cli (incl. the 417-line server_drain_test.go), src/cmd/tollgate-cli (drain flow + confirm tests). gofmt/go vet clean.
• Diff walk (per PR-REVIEW.md): the canonical-mint-identity refactor is the right shape — one normalizeMintURL as the single identity function for registry keys, AddMint inputs, and MintURLMatches, which structurally closes the two-spellings-of-one-mint duplicate-balance class from tollgate wallet drain cashu #375; the read-side alias merge in GetAllMintBalances (max-view per alias group, DB never rewritten) keeps old wallets drainable without migration; the fsync'd journal + partial-results semantics match the drain-safety goals in the plan doc.
• Base freshness: 1 commit behind main — no rebase hazard (fork point is post-mint-health: payment-path MarkUnreachable suppresses the degraded-mode transition #401).
• The CHANGELOG rewrite is large but reconciles to the drain-safety narrative; entries link the PR.

No changes requested from our side; merge-ready pending review sign-off.

[Amperstrand]

#443 addressed on this branch (fc9954f) — disposition: observability + documentation, not a build-tag gate.

Reasoning for the choice: the harness sets TOLLGATE_TEST_CONFIG_DIR via t.Setenv in dozens of subpackage tests that run both with and without -tags testenv, so gating the branch behind the tag would break tagless runs (and silently repoint them at /etc/tollgate on dev machines) — too much blast radius for hygiene-only value given no known exploit path (it requires service-environment control, at which point the game is mostly over, as the issue notes).

What landed instead:

• drainJournalPath() and socketPath() now print WARNING: TOLLGATE_TEST_CONFIG_DIR is set — … redirected to <dir> on every honor — a stray drop-in becomes visible in logread instead of a mystery of split state.
• The operator guide documents the variable, the token-sink implication, and the remedy.

If an exploit class ever materializes, the tag-gate remains the escalation path (the commit message records the tradeoff).

Touched-module battery green: src/cli and src/cmd/tollgate-cli under -race -count=1, gofmt/go vet clean.

[Amperstrand]

The go-test.yml red at 01:59Z on this head (cc269f7a) is runner flake, not the code: the identical gate just passed locally on the identical commit — gofmt -l empty, go vet/go build clean, and go test -race -count=1 -tags testenv ./... from src/ green (root module ok, 17.3 s). Same profile as last night's known runner flakiness (setup-go clone timeouts out of the act container — visible in the main-branch test.yml failures too, with sibling go-test legs green in the same runs and content-reading jobs all passing). Nothing to address here.