Skip to content

fix(j1939tp): order a cancelled CM abort before the next RTS - #454

Merged
dborgards merged 1 commit into
mainfrom
fix/j1939tp-confirmed-cancel-order-356
Oct 25, 2026
Merged

dborgards merged 1 commit into
mainfrom
fix/j1939tp-confirmed-cancel-order-356

Conversation

@dborgards

Copy link
Copy Markdown
Owner

What does this change?

Cancelling a CM send after its RTS was confirmed or answered immediately released the destination. The queued successor's RTS could therefore reach the bus service before the cancelled send's Connection Abort, because each control frame is dispatched on its own pool work item.

Every cancelled CM send now keeps the destination reserved until its Abort has crossed the service hand-off. An unsettled RTS still delays its Abort; an already settled RTS dispatches the Abort immediately through the same guarded path. The caller's cancellation completes without waiting for that hand-off. No public API change.

Closes #356. Its other actionable findings were resolved by #390, #414 and #418; the remaining SAE source-address questions are tracked separately in #422 and remain open. Refs #362 for the test helper's removal of a wall-clock hold limit.

Type of change

  • feat — new behaviour (minor release)
  • fix / perf — bug or performance fix (patch release)
  • docs / test / refactor / chore / ci — no release
  • Breaking change

Checklist

  • Build with -p:CI=true, test (net10.0), dotnet format --verify-no-changes, and pack + eng/verify-packages.py all pass locally
  • Public API changes are documented with XML comments — no public API change; README describes cancellation ordering
  • New behaviour is covered by a test
  • The requirement or ADR this relates to is referenced — FR-TP-031, FR-TP-034

Validation

  • Regression: A_Cm_Send_Cancelled_After_The_Peer_Answered_Hands_Off_Its_Abort_Before_The_Next_Rts uses CTS(0) and an observed virtual T4 arm to establish that the RTS was answered, then holds the Abort inside the service hand-off. The next send must remain queued until that hold is released. The hold has no wall-clock expiry and is released in finally/service disposal.
  • Before the fix the regression fails with queued count 0 instead of 1; with the fix it passes. All four targeted CM cancellation cases pass.
  • Release build: 0 warnings/errors. Full net10.0 suite with coverage: 2882/2882 passed. Format verification passed; fresh pack verified all 9 packages.
  • Local Cobertura: both modified executable branches have 100% condition coverage (2/2 each); no modified executable line is missing coverage. All class entries for the file were inspected.
  • Rebased over ci: disable inactive review flags while keeping zero required reviews #453 afterwards. The only difference from the fully tested tree is .github/rulesets/main.json; production and test sources are identical. All 50 verification-script tests pass on the rebased tree.
  • net48 execution is left to Windows CI.

@cursor

cursor Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes connection-mode TX cancellation and per-destination serialization on the actor loop; wrong ordering could confuse peers, but behavior is regression-tested and scoped to CM cancel paths.

Overview
Fixes CM send cancellation ordering when the RTS is already on the wire (confirmed or answered by the peer) but a queued successor is waiting on the same destination.

Previously, cancelling in that state could release the destination immediately and send Connection Abort on a separate pool work item, so the next send’s RTS could reach the bus service before the cancelled session’s Abort (#356).

All cancelled CM sends now keep the destination reserved via _abortsAwaitingRts until the Abort has crossed the service hand-off; the caller’s task still completes right away. If the RTS is not yet settled, Abort still waits for OnRtsSettled; if it is already settled, OnRtsSettled is invoked immediately on the same guarded path instead of sending Abort inline.

README documents this behavior. A regression test covers cancel after CTS(0) with Abort held in HeldConfirmationBusService; the test helper’s hand-off wait no longer uses a 30s wall-clock timeout.

Reviewed by Cursor Bugbot for commit 8983ade. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 25, 2026 •

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 ✅ Completed 2026-10-25T14:21:07.017860Z 8983ade 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.

@codecov

codecov Bot commented Oct 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@dborgards
dborgards merged commit 4801aee into main Oct 25, 2026
16 checks passed
@dborgards
dborgards deleted the fix/j1939tp-confirmed-cancel-order-356 branch October 25, 2026 14:33
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.

J1939-TP: minor findings (T-2..T-8), the untested T2 re-arm, and doc drift from the 2026-10-21 deep review

1 participant