Skip to content

fix(peer): send fatal-error answers before the connection closes - #149

Merged
gomaja merged 1 commit into
mainfrom
fix/i147-fatal-error-answer
Oct 17, 2026
Merged

gomaja merged 1 commit into
mainfrom
fix/i147-fatal-error-answer

Conversation

@gomaja

@gomaja gomaja commented Oct 17, 2026 •

Copy link
Copy Markdown
Owner

Summary

When a request arrived with a fatal message error (an unsupported version, or a Message Length shorter than the header or not a multiple of 4), Manager queued the error answer and returned to the Server reader. The reader then closed the transport, usually before the session writer had sent the answer. For a fatal error, Manager now returns only once that answer has been written or has failed, the session has ended, or Timers.Closing has elapsed since the decision. The answer therefore reaches the peer before the connection closes. The MessageErrorHandler contract now states this rule.

Closes #147.

Behaviour

Answers delivered before EOF, Manager accepted and dialed:

Case Before After
Open over TCP: 5011, 5015 for a short length, 5015 for an unaligned length (24 runs each) 0–3 of 24 24 of 24
Accepted, CER with the same header errors 0 of 72 72 of 72
Open over TLS (12 runs per case) 1–5 of 12 12 of 12
Open over SCTP (12 runs per case) 0–4 of 12 12 of 12
Direct fatal-error call on a valid header, 5014 (24 runs each) 0–1 of 24 24 of 24
800 established TCP connections 248 answered 800 answered, no reply_failure

Unchanged:

  • StateMachine and Client, which already answered.
  • A dialed connection waiting for CEA, where RFC 6733 §5.6 selects no answer.
  • A truncated body, which ends in EOF rather than a message error.
  • Non-fatal errors.

Testing

  • New public tests: TCP, TLS and SCTP through Manager accepted and dialed, with StateMachine and Client as the parity reference. SCTP runs in Linux containers.
  • Deterministic testing/synctest tests: every way the wait ends (written, failed, short write, skipped, terminal disposal, session end, the Closing bound), plus a peer that stops reading.
  • Mutation check: removing the wait makes every new public test family fail.
  • Packet captures: over TCP and SCTP, accepted and dialed, the answer leaves before FIN or SCTP SHUTDOWN.
  • Allocations: BenchmarkInboundMessages in peer and sm shows identical B/op and allocs/op to main in all 14 cases.
  • Local validation:
    • every module on Go 1.27.2 and 1.26.9;
    • the race suite, with 20 repetitions of the new tests on both toolchains;
    • vet, staticcheck, golangci-lint, govulncheck and the secret scan;
    • the Windows build and CodeQL;
    • apidiff: no API change in 48 packages;
    • all 13 SCTP wire-harness variants.
  • Also fixed: TestAcceptedRestartFreshHandshake now checks the replacement's admission after its DWR/DWA exchange. The handshake notification it waited on can arrive before admission is released, which made it fail intermittently under the race detector.

Summary by cubic

Fixes fatal-error answers being lost when the connection closes. Manager now waits for the error answer before the Server closes the transport, so peers get the required 5011/5015/5014 answers instead of a dropped connection.

Previously Manager queued the answer and returned, letting the reader close the connection first — on 800 established TCP connections, 552 received no answer. Now HandleMessageError returns only after the answer is written, fails, the session ends, or Timers.Closing elapses since the decision, so a peer that stops reading can't hold the reader past that bound. Non-fatal errors and decisions without an answer don't wait.

Contract

  • MessageErrorHandler handlers must finish any fatal-error answer before returning; documented in diam/server.go.

Also fixed

  • TestAcceptedRestartFreshHandshake checks the replacement's admission after its DWR/DWA exchange, since the handshake notification can arrive before admission is released.
  • New tests cover TCP, TLS, and SCTP, accepted and dialed, including mutation and allocation checks.

Closes #147.

Written for commit 63ee7e3. Summary will update on new commits.

View guided diff Turn on auto-fix

For a request with a fatal message error, such as an unsupported
version or an invalid message length, Manager queued the required
error answer and returned to the Server reader, which then stops
reading and closes the transport. The session writer raced that close,
so the answer was usually lost and observers often saw a second
reply_failure event: in one run of 800 established TCP connections,
552 received no answer. StateMachine and Client write the answer before
they return.

For a fatal error, Manager.HandleMessageError now returns only after
the answer its decision queued has been written, skipped or has failed,
after the session has ended, or once Timers.Closing has elapsed since
the decision. The Server's close therefore follows the answer, and a
peer that stops reading cannot hold the reader past that bound.
Non-fatal errors and decisions without an answer do not wait, and
valid messages allocate nothing more.

Behaviour changes, for accepted and dialed Manager connections over
TCP, TLS and SCTP:

- A request in Open with an unsupported version gets its 5011 answer,
  and one whose Message Length is shorter than the header or not a
  multiple of 4 gets its 5015 answer, before the connection closes
  (RFC 6733 sections 3 and 7.1.5, verified erratum 3805).
- On an accepted connection, a CER with such a header error gets its
  error answer before the connection closes.
- Observers see one event with Reply set to ProtocolReplyQueued for an
  answer that is written, and a ProtocolReplyFailure event only when
  the write itself fails.
- A direct HandleMessageError call with a fatal error for a message
  with a valid header, such as an invalid AVP length (5014), also
  returns after its answer is written.

The MessageErrorHandler contract now says that after a fatal error the
Server reads no further and closes the connection when the call
returns, so a handler must finish any answer it wants delivered before
returning.

TestAcceptedRestartFreshHandshake checks the replacement's admission
after its DWR/DWA exchange instead of right after the handshake
notification, which can arrive before admission is released.

Closes #147.
Copilot AI balanced review requested due to automatic review settings October 17, 2026 05:59
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-17T05:59:10.825407Z 63ee7e3 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Oct 17, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: bde97983-6eb9-4822-85f0-8221bc91cc53
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gomaja
gomaja merged commit dd84d47 into main Oct 17, 2026
2 checks passed
@gomaja
gomaja deleted the fix/i147-fatal-error-answer branch October 17, 2026 06:04
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.

peer: Manager often closes before sending the error answer to a request with a fatal header error

2 participants