sign: eliminate TSA response deadlocks, send-on-closed-channel panics, and invalid --max values #2

New issue

Closed

opened 2026-07-21 17:20:15 +02:00 by heiko · 1 comment

heiko commented

2026-07-21 17:20:15 +02:00

Owner

Problem

The response fan-out/fan-in logic in sign.go counts all entries in TSAs, but starts one goroutine only for entries whose boolean is true:

timestamps := make(chan *timestamp, 10)
defer close(timestamps)
for endpoint, use := range TSAs {
    if !use { continue }
    go func(...) { timestamps <- result }(...)
}
...
for range len(TSAs) {
    ts := <-timestamps
}

Consequences:

If every enabled TSA fails, all real responses are consumed and the receiver then blocks forever waiting for disabled TSAs that never started.
--max is validated against total map entries, not enabled endpoints. Zero and negative values are accepted and have nonsensical/nonterminating behavior.
The receiver owns defer close(timestamps) and may return after enough successes while slower senders are still active; a later send can panic.
Channel capacity is hard-coded to 10 while the map currently has more entries.
Outstanding requests are not canceled after the requested number of successes.

Checklist: #62 (goroutines need a stop plan), #67 (channel sizing/ownership), #73 (structured goroutine groups).

Proposed fix

Build a deterministic enabled []tsaConfig first.
Reject --max < 1 and --max > len(enabled) before launching work.
Derive request contexts from the Cobra command context and a shared cancellation context.
Have each worker send exactly one result, or use a WaitGroup/errgroup and close the results channel only after all senders finish.
Never close a channel from the receiving side while senders may still run.
Cancel outstanding requests once enough valid successes are accepted.
If all workers finish before --max successes, return an aggregate error rather than blocking.
Preserve deterministic diagnostics where practical; map iteration should not define policy.

Acceptance criteria

All TSAs failing returns within the configured deadlines.
One fast success plus one slow worker cannot panic after return.
--max=0, negative values, and values above the enabled count fail immediately.
Cancellation stops pending workers.
Tests cover all-fail, mixed success/failure, early success, slow worker, and race execution (go test -race).

Reviewed against 948b7a9 on master (Go 1.26.2).

## Problem The response fan-out/fan-in logic in `sign.go` counts all entries in `TSAs`, but starts one goroutine only for entries whose boolean is true: ```go timestamps := make(chan *timestamp, 10) defer close(timestamps) for endpoint, use := range TSAs { if !use { continue } go func(...) { timestamps <- result }(...) } ... for range len(TSAs) { ts := <-timestamps } ``` Consequences: - If every enabled TSA fails, all real responses are consumed and the receiver then blocks forever waiting for disabled TSAs that never started. - `--max` is validated against total map entries, not enabled endpoints. Zero and negative values are accepted and have nonsensical/nonterminating behavior. - The receiver owns `defer close(timestamps)` and may return after enough successes while slower senders are still active; a later send can panic. - Channel capacity is hard-coded to 10 while the map currently has more entries. - Outstanding requests are not canceled after the requested number of successes. Checklist: #62 (goroutines need a stop plan), #67 (channel sizing/ownership), #73 (structured goroutine groups). ## Proposed fix - Build a deterministic `enabled []tsaConfig` first. - Reject `--max < 1` and `--max > len(enabled)` before launching work. - Derive request contexts from the Cobra command context and a shared cancellation context. - Have each worker send exactly one result, or use a `WaitGroup`/`errgroup` and close the results channel only after all senders finish. - Never close a channel from the receiving side while senders may still run. - Cancel outstanding requests once enough valid successes are accepted. - If all workers finish before `--max` successes, return an aggregate error rather than blocking. - Preserve deterministic diagnostics where practical; map iteration should not define policy. ## Acceptance criteria - All TSAs failing returns within the configured deadlines. - One fast success plus one slow worker cannot panic after return. - `--max=0`, negative values, and values above the enabled count fail immediately. - Cancellation stops pending workers. - Tests cover all-fail, mixed success/failure, early success, slow worker, and race execution (`go test -race`). Reviewed against `948b7a9` on `master` (Go 1.26.2).

heiko referenced this issue

2026-07-21 17:20:21 +02:00

rfc822: remove unconditional parser diagnostics, unused bookkeeping, and stale commented implementations #12

heiko referenced this issue

2026-07-21 17:20:24 +02:00

development: keep ignored scratch experiments out of ./... package discovery #14

heiko commented

2026-07-21 23:23:31 +02:00

Author

Owner

Signing now emits exactly --max successes: the accept loop breaks at --max, cancels the request context, and waits for the cancelled workers before returning. Full acceptance matrix covered (all-fail, mixed, early-success/slow-worker, cancellation, --max 0/negative/over-count, race).

Closing commit: 2a8c44613f5c

Note: currently on local master only; the link resolves once these commits are pushed (origin/master is still at 948b7a9).

Signing now emits exactly `--max` successes: the accept loop breaks at `--max`, cancels the request context, and waits for the cancelled workers before returning. Full acceptance matrix covered (all-fail, mixed, early-success/slow-worker, cancellation, `--max` 0/negative/over-count, race). Closing commit: [`2a8c44613f5c`](https://forgejo.schlittermann.de/heiko/mail-seal/commit/2a8c44613f5c96a8281baad6c6927a3dd14d1d6f) _Note: currently on local `master` only; the link resolves once these commits are pushed (`origin/master` is still at `948b7a9`)._

heiko closed this issue

2026-07-21 23:23:31 +02:00

heiko referenced this issue from a commit

2026-07-22 11:18:25 +02:00

test: cover TSA concurrency criteria from issue #2

heiko referenced this issue from a commit

2026-07-22 11:18:25 +02:00

merge: integrate signing/verification work onto rfc822 parser fixes

heiko referenced this issue from a commit

2026-07-22 11:18:25 +02:00

Merge branches 'issues/4-9-11-13-tsp-tcp', 'issues/14-scratch-layout' and 'issues/3-12-rfc822-parser'

heiko referenced this issue from a commit