Fix IEEE 802.11 ADDBA transaction handling - #1129
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| [Config MacQosWithTransactionalBlockAck] | ||
| description = "Exercises successful and timed-out ADDBA transactions" | ||
| extends = MacQosWithoutAggregation | ||
| sim-time-limit = 3s |
There was a problem hiding this comment.
🔴 Existing Block Ack example loses its radio medium setting and can abort with an error
The new example configuration is inserted (at examples/wireless/qos/omnetpp.ini:86) between the previous last configuration and the trailing radio-medium setting lines, so the setting that used to belong to the Block Ack example silently moves to the new example.
Impact: The pre-existing Block Ack example now runs with the strict default and can stop with a runtime error when two transmissions start at the same moment; its recorded results change too.
How the ini section boundary shifts the setting
In an omnetpp.ini file, keys belong to the section they textually follow. Before this PR, the lines
# radio medium
*.radioMedium.sameTransmissionStartTimeCheck = "ignore"
were the last lines of [Config MacQosWithBlockAck]. The new [Config MacQosWithTransactionalBlockAck] block is inserted above them, so those two lines now belong to the new config and MacQosWithBlockAck falls back to the NED default error (src/inet/physicallayer/wireless/common/medium/RadioMedium.ned:44), which raises a runtime error via src/inet/physicallayer/wireless/common/medium/RadioMedium.cc:484-497. Note also that MacQosWithTransactionalBlockAck extends MacQosWithoutAggregation, so the new config would not have inherited the setting either; this reassignment is accidental. The existing fingerprint row for MacQosWithBlockAck in tests/fingerprint/examples.csv:661 is left unchanged.
Prompt for agents
The two trailing lines of examples/wireless/qos/omnetpp.ini ('# radio medium' and '*.radioMedium.sameTransmissionStartTimeCheck = "ignore"') were part of [Config MacQosWithBlockAck] because ini keys belong to the preceding section. The newly added [Config MacQosWithTransactionalBlockAck] section was inserted before them, so MacQosWithBlockAck lost the setting and now uses the RadioMedium default 'error', which can abort the simulation. Restore the setting to MacQosWithBlockAck (e.g. append the new config after those lines, or explicitly duplicate/hoist the radioMedium assignment where it is actually needed).
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!hasFrameToTransmit(ac)) { | ||
| EV_DETAIL << "Releasing channel because no eligible frame is available.\n"; | ||
| edcaf->releaseChannel(this); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Traffic queues can stall after the radio is handed back without sending anything
When the radio is granted to a queue that currently has nothing sendable, the radio is handed back immediately (edcaf->releaseChannel(this) at src/inet/linklayer/ieee80211/mac/coordinationfunction/Hcf.cc:229) before the other queues that lost the simultaneous grant are told to back off and retry, so those queues stop contending.
Impact: Packets waiting in other priority queues can sit unsent until unrelated new traffic arrives, causing extra delay or loss.
Skipped internal-collision recovery
When several access categories reach their transmit time simultaneously, only the highest-priority EDCAF gets channelGranted(); the losers are marked as internal collisions (src/inet/linklayer/ieee80211/mac/contention/EdcaCollisionController.cc:33-46) and do not get their callback (src/inet/linklayer/ieee80211/mac/channelaccess/Edcaf.cc:127-133). Their recovery (backoff update, drop on retry limit, and edcaf->requestChannel(this)) is performed exclusively by the winner in Hcf::handleInternalCollision() (src/inet/linklayer/ieee80211/mac/coordinationfunction/Hcf.cc:229-297 region). The new early return at line 230 executes before edca->getInternallyCollidedEdcafs() is consulted, so the collided EDCAFs' contention has already ended and nobody restarts it. The early return is reachable because frame eligibility can change between requesting the channel and being granted it (a pending ADDBA transaction makes all same peer/TID QoS frames ineligible via Hcf::hasFrameToTransmit(AccessCategory)).
Prompt for agents
In Hcf::channelGranted(), the new early return releases the channel when no eligible frame exists, but it bypasses the internal-collision handling that follows (edca->getInternallyCollidedEdcafs() / handleInternalCollision()). EDCAFs that lost the simultaneous grant rely on the winner to run their recovery procedure and re-request the channel, so they stop contending. Handle the internally collided EDCAFs (and emit edcaCollisionDetectedSignal) before releasing the channel and returning.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Marked as Outdated by Devin
| else { | ||
| // TODO send a new one? | ||
| terminateAgreement(addbaResp->getTransmitterAddress(), addbaResp->getTid()); | ||
| scheduleAddbaResponseTimer(callback); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
🔴 Traffic to a peer that refuses Block Ack is throttled to one frame per timeout period
The Block Ack setup record is deleted (terminateAgreement(...) at src/inet/linklayer/ieee80211/mac/blockack/OriginatorBlockAckAgreementHandler.cc:200, and by the timeout cleanup at lines 60-69) when the peer refuses or never answers, so the very next data frame starts a brand new setup attempt that again blocks all further frames for that peer and priority.
Impact: Data sent to a receiver that does not support Block Ack is repeatedly held for the whole setup timeout, collapsing throughput and overflowing the queue.
Retry loop between eligibility gating and agreement deletion
While a setup is pending, Hcf's frame-eligibility function (src/inet/linklayer/ieee80211/mac/coordinationfunction/Hcf.cc:63-66) makes every QoS data frame to that receiver/TID ineligible, so they stay queued. On rejection (processReceivedAddbaResp) or on timeout (addbaResponseTimeoutExpired) the agreement object is erased entirely. processTransmittedDataFrame (src/inet/linklayer/ieee80211/mac/blockack/OriginatorBlockAckAgreementHandler.cc:176-183) creates a new agreement whenever getAgreement() returns null and isAddbaReqNeeded() is true; the default policy's isAddbaReqNeeded() only checks frame type and length (src/inet/linklayer/ieee80211/mac/blockack/OriginatorBlockAckAgreementPolicy.cc:38-41), so it is true for every QoS data frame. The result is: one frame goes out, a new ADDBA is issued, all following frames are held for addbaFailureTimeout (default 1s), the setup fails again, and the cycle repeats indefinitely. Previously the pending agreement was kept forever, so only one ADDBA was ever sent and data flowed normally with normal ACK. Consider remembering failed peers/TIDs (or a retry limit/backoff) so ADDBA is not re-attempted for every frame, and/or not gating frames when no agreement can be established.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Asked Devin again, his answer: "This flag no longer applies. The PR added exactly the retry-backoff mechanism the flag recommends."
| if (agreement && agreement->isPending() && agreement->getDialogToken() == addbaReq->getDialogToken()) { | ||
| if (agreement->getAddbaResponseDeadline() < 0) { | ||
| auto addbaFailureTimeout = blockAckAgreementPolicy->computeAddbaFailureTimeout(); | ||
| if (addbaFailureTimeout <= 0) | ||
| throw cRuntimeError("ADDBA failure timeout must be greater than zero"); | ||
| agreement->setAddbaResponseDeadline(simTime() + addbaFailureTimeout); | ||
| } | ||
| agreement->setIsAddbaRequestSent(true); | ||
| scheduleAddbaResponseTimer(callback); | ||
| } |
There was a problem hiding this comment.
🟡 Frames can be held forever if the Block Ack setup request is never sent on air
The deadline for giving up on a Block Ack setup is only armed when the request frame actually goes on air (agreement->setAddbaResponseDeadline(...) at src/inet/linklayer/ieee80211/mac/blockack/OriginatorBlockAckAgreementHandler.cc:222), so a setup whose request is discarded before transmission never expires.
Impact: All data to that receiver and priority stays stuck in the queue indefinitely, silently dropping the flow.
Path where the request is dropped before transmission
processTransmittedDataFrame creates the agreement and hands the ADDBA Request to processMgmtFrame, which enqueues it. From that moment the eligibility function in Hcf::initialize (src/inet/linklayer/ieee80211/mac/coordinationfunction/Hcf.cc:63-66) holds all QoS data for that receiver/TID because isAddbaResponsePending() is true. The deadline, and hence computeEarliestAddbaResponseDeadline() (which requires getIsAddbaRequestSent()), only becomes active in processTransmittedAddbaReq. If the management frame is dropped before it is ever transmitted — e.g. retry-limit reached during internal collision handling in Hcf::handleInternalCollision (src/inet/linklayer/ieee80211/mac/coordinationfunction/Hcf.cc:270-286) — the agreement stays pending with deadline -1 and is never cleaned up by addbaResponseTimeoutExpired. Arming the deadline at agreement creation (or removing the agreement when its ADDBA Request is dropped) would avoid the permanent stall.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Marked as Outdated by Devin
| Packet *packet = nullptr; | ||
| for (int i = 0; i < pendingQueue->getNumPackets(); i++) { | ||
| auto candidate = pendingQueue->getPacket(i); | ||
| if (isFrameEligible(candidate)) { | ||
| pendingQueue->removePacket(candidate); | ||
| packet = candidate; | ||
| break; | ||
| } | ||
| } | ||
| ASSERT(packet != nullptr); |
There was a problem hiding this comment.
🟡 Queue statistics for outgoing Wi-Fi frames are lost
Frames are now taken out of the transmit queue with a plain removal call (pendingQueue->removePacket(candidate) at src/inet/linklayer/ieee80211/mac/originator/OriginatorMacDataService.cc:66, and the same in the QoS variant) instead of the normal dequeue path, so the queue no longer reports the packet as served.
Impact: Recorded queueing-time and dequeue statistics for all Wi-Fi transmit queues disappear or change, even in configurations unrelated to Block Ack.
Difference between dequeuePacket() and removePacket()
PacketQueueBase::dequeuePacket() calls pullPacket() which stamps the queueing time tag, inserts the packet event and emits packetPulledSignal (src/inet/queueing/queue/PacketQueue.cc:117-133), which feeds @statistic[queueingTime]. removePacket() only emits packetRemovedSignal (src/inet/queueing/queue/PacketQueue.cc:135-143). The new selection loops in both OriginatorMacDataService::extractFramesToTransmit and OriginatorQosMacDataService::extractFramesToTransmit always use removePacket(), including the common case where the selected packet is the queue head and no eligibility filtering is in effect (the non-QoS DCF service never gets an eligibility function). Using dequeuePacket() when the chosen candidate is the head would preserve the previous statistics behaviour.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Marked as Outdated by Devin
Track each originator ADDBA setup with an exact local transaction ID, start the response timeout only after the complete request is transmitted, and apply a separate retry backoff after timeout, refusal, or discard. Cancel all queued and in-progress fragments belonging to failed or completed transactions so stale requests cannot block or later re-establish state. Establish recipient Block Ack agreements when a request is accepted, remove response-packet staging, reset reorder buffers on renegotiation, and publish distinct agreement-change observations. Preserve internal-collision recovery and materialize frames only at DCF grants and HCF TXOP continuation boundaries so availability queries remain side-effect free. Add typed queue drop callbacks and arbitrary-packet dequeue propagation through queues, flows, and schedulers. Restore QueueingTimeTag, PEK_QUEUED, and packetPulled accounting exactly once while keeping cancellation on removal/drop semantics. Extend the focused regression with real fragmentation and transaction cancellation, reorder reset, queue accounting and overflow callbacks, HCF continuation, and legacy DCF channel-grant coverage. Validation: release and debug builds; focused Ieee80211AddbaTransaction unit test; MacDcf and MacEdca smoke simulations; architecture and WLAN reviews; 22 related fingerprints with 15 unchanged and 7 explained Block Ack trajectory changes. Fingerprint baselines are intentionally unchanged.
Defer ADDBA initiation until the triggering QoS MPDU is acknowledged and its final fragment completes. This preserves the negotiated starting sequence number and prevents an outstanding retry or remaining fragment from falling below the recipient reorder window. Resume eligible channel access when a dropped setup frame terminates a pending transaction, and cancel tagged ADDBA requests when DELBA tears down pending state. Restore DCF pending-frame materialization and defensively handle internal collisions with no selectable frame. Use the configured EDCAF count for HCF queue traversal, track packet-drop callback registration, and unregister callbacks during teardown. Extend the focused unit test with ACK timing, fragmented MSDU, DELBA cancellation, queue wakeup, and DCF materialization coverage. Validation: release build and focused Ieee80211AddbaTransaction_1 unit test pass; git diff --check passes. The full unit and fingerprint suites were not run.
Keep ADDBA setup requests and same-peer/TID data synchronized with the pending originator agreement. Harden cancellation, retry cleanup, DELBA teardown, reordering ownership, and in-progress frame disposal so stale setup traffic cannot be double-freed or leave protocol state behind. Add an HCF-owned per-access-category eligibility index. Track exact queue departures through typed lifecycle callbacks and rebuild eligibility only when Block Ack state changes, making routine channel-access availability checks independent of pending queue depth. Preserve configurable queue-provider policy during selective extraction and A-MSDU formation. Add predicate-aware Priority, WRR, Label, flow, leaf, and compound extraction; batch overflow notifications to avoid reentrant victim selection; and conservatively skip pre-extraction aggregation through transforming packet flows. Document the incompatible queue contracts and DELBA standards trace. Expand focused coverage for ADDBA lifecycle transitions, direct and RTS failure cleanup, custom and compound queues, scheduler accounting, shared buffers, A-MSDU processing, and the no-scan eligibility invariant. Validated with release/debug builds, focused unit tests in both modes, runtime-clean affected fingerprint simulations, and an independent INET/WLAN architecture review.
Prevent predicate-based extraction from bypassing closed gates or implicit guard bands by validating the exact upstream candidate before dequeueing it. Drain PacketQueue instances completely during bulk removal and detach only the queue's own packets from shared buffers so removal callbacks remain complete and isolated. Transfer buffered Block Ack frames to the recipient data service during reset, emit one packet-drop signal per discarded MPDU, and reclaim cancelled out-of-sequence ADDBA frames immediately. Clear both short and long retry state on terminal frame discard, and make scheduler extractor requirements explicit while preserving PriorityScheduler's nullable collection accounting. Add focused regression coverage for closed and guarded gates, multi-packet and shared-buffer removal, reorder-reset drop accounting, idle ADDBA cancellation, retry-state cleanup, and extractor-only priority inputs. Validated with the focused Ieee80211AddbaTransaction_1 unit test in release and debug modes.
Reset recipient-side Block Ack reordering when a locally transmitted recipient DELBA removes an agreement. Keep the removed agreement alive through HCF notification so teardown observers receive valid state, and remove the unused ADDBA-response-sent flag. Return removed recipient agreements from the transmitted-DELBA handler and cover teardown plus same-peer/TID reestablishment with a reorder-buffer regression. The test proves stale sequence state and buffered fragments do not leak into the replacement session. Report shared PacketBuffer removals to PacketQueue observers exactly once. A dedicated pre-drop detach callback preserves batch removal semantics: ordinary removals report REMOVED, overload victims report DROPPED, and all selected victims are detached before drop callbacks can re-enter queue selection. Add focused coverage for direct shared-buffer removal, overload callback reasons, recipient DELBA cleanup, and fresh reorder state after renegotiation. Validation: - Release and debug builds pass. - Focused ADDBA unit tests pass in release and debug. - The full release unit suite passes 78/90; the 12 failures are unrelated pre-existing clock and TCP cases. - Focused Block Ack fingerprint mismatches are unchanged by an old-vs-new HCF channel-resumption A/B, so fingerprint baselines remain untouched. - Architectural review reports no new violations.
3a40816 to
2a389ec
Compare
Return removed originator agreements from transmitted DELBA processing so HCF can emit balanced agreement lifecycle signals only for agreements that were actually established. Treat successful ADDBA responses as established before applying a local policy veto, then queue an initiator DELBA with END_BA. Handle transmitted and pre-transmission-aborted teardown frames idempotently, preserve retry suppression, and restore frame eligibility without leaving a blocked TID. Make DROPPED and REMOVED unsent ADDBA requests terminal while keeping DEQUEUED as an ownership transfer. Count only actionable EDCA internal collisions and retain release/end-TXOP/all-AC resumption ordering without restarting active contention. Rename addbaFailureTimeout to addbaResponseTimeout, document the API and configuration migration, and add focused debug/release coverage for lifecycle signals, queue removal, local veto teardown, retry backoff, collision filtering, and channel-access resumption. Validation: debug and release builds pass; Ieee80211AddbaTransaction_1 passes in both modes; git diff --check passes. Fingerprint baselines are intentionally unchanged pending explicit acceptance of the attributed EDCAF timing shift.
Keep nullable packet collection and extractor capabilities in WrrScheduler, LabelScheduler, and PriorityScheduler. Validate aggregate and predicate operations lazily and consistently so ordinary passive sources remain valid scheduler inputs. Make A-MSDU selection candidate-aware across flow modules and non-order-preserving schedulers. Remove selected members through exact predicate dequeues, preserve provider accounting, and revalidate aggregation-critical frame fields before building the aggregate. Immediately roll back successful ADDBA agreements rejected by local policy while retaining Normal Ack data service. Track best-effort DELBAs by transaction, reject stale generations, handle fragmented transmit and abort lifecycles, and cancel obsolete queued or in-progress teardown packets before replacement setup. Document the updated scheduler and aggregation contracts, update WHATSNEW, and add focused regression coverage for scheduler capabilities, flow and reverse-priority aggregation, agreement signal ordering, stale teardown disposal, fragmentation, and queue cleanup.
Keep transaction-tagged initiator DELBA frames eligible until a final-fragment acknowledgement or terminal abort, allowing failed management transmissions to follow the normal retry and retry-limit paths. Make teardown abort outcomes explicit in HCF and clean up sibling frames and acknowledgement state exactly once. Preserve same-flow MSDU ordering during A-MSDU selection, make extraction contract validation exception-safe, propagate destructive drops through nested compound queues exactly once, reject unsupported PacketBuffer ownership before mutation, and make fragment tag propagation robust. Document the scheduler aggregate-query compatibility change. Add focused coverage for DELBA retry, acknowledgement and retry-limit cleanup; malformed packet extractors; nested queue callbacks; PacketBuffer ownership; scheduler capabilities; and fragment tag propagation. Validated with release and debug builds, focused Ieee80211AddbaTransaction_1 tests in both modes, git diff --check, and independent architecture and IEEE 802.11 semantic review. Fingerprint baselines are intentionally unchanged.
Propagate MAC duplicate detection from the recipient QoS data service into HCF so retransmitted management frames are not processed as new negotiations. Cache the exact ADDBA response per originator and TID and replay it for a recognized duplicate while preserving the existing agreement, reorder window, buffered MPDUs, inactivity timer, and agreement signals. Fresh MAC identities still perform normal renegotiation, including when their ADDBA parameters are unchanged. Clear replay state during agreement teardown and avoid retaining responses for initial rejected requests without an agreement. Add focused transaction coverage for accepted and rejected requests, duplicate response replay, genuine renegotiation, reorder-buffer preservation, DELBA handling, and cache lifecycle.
Forward REMOVED callbacks from descendant queues through CompoundPacketQueueBase so observers are notified whenever a packet leaves the enclosing logical queue, including leaf-initiated and shared-buffer removals. Track the packet currently removed at the compound boundary with a scoped save-and-restore guard. Suppress only the matching descendant callback during boundary removal and overflow victim detachment, preserving exactly-once delivery and the intended DROPPED reason while allowing nested or reentrant removals of other packets to propagate. Extend the ADDBA transaction unit coverage for direct and nested compounds, bulk and shared-buffer removal, boundary remove/dequeue/pull paths, reentrant removal, and compound capacity drops. Validation: debug build; focused Ieee80211AddbaTransaction_1 unit test; focused PriorityQueue and EthernetQosQueue fingerprints; focused architecture check; independent semantic review.
Clarify agreement ownership, harden reentrant cleanup, restore queue extraction animation, and add focused regressions for the reviewed edge cases.
|
The last commit contains the updated fingerprints mentioned in the following report. Responses to the reviewer (Devin)Date: 2026-08-22 Reviewed range: Latest fingerprint validation HEAD: Scope: all twelve supplied final-review comments ConclusionThe final review contains one useful defensive-hardening request, two intentional model/API-scope changes that should remain documented, one confirmed but non-normative reorder-buffer data-loss concern, one performance comment already addressed at the current HEAD, five correct informational observations, and two defect premises that do not hold for the current implementation. No supplied comment demonstrates a current-path merge blocker. The two worthwhile follow-ups are:
The eight directly related debug fingerprint rows whose expectations were updated in
Detailed responses1. Reentrancy of
|
| Test row | Previously recorded | Calculated by the full-suite run |
|---|---|---|
tests/fingerprint/examples.csv:661 — MacQosWithBlockAck |
8306-3cd3/tplx;26d1-9165/~tNl;e0fc-7553/~tND |
040e-6428/tplx;557f-bcad/~tNl;68e6-5a6d/~tND |
tests/fingerprint/examples.csv:662 — MacQosWithTransactionalBlockAck |
c4ff-d71f/tplx;68ba-2827/~tNl;9a98-cc02/~tND |
b608-4cad/tplx;6f80-132d/~tNl;9b78-3d6c/~tND |
tests/fingerprint/showcases.csv:202 — NoFragmentation |
aa2d-5d35/tplx;2094-1f2a/~tNl;1470-1e1b/~tND |
7ecf-66f5/tplx;193b-2155/~tNl;fa4f-92fc/~tND |
tests/fingerprint/showcases.csv:203 — Fragmentation |
7ae9-e07d/tplx;db8b-3b81/~tNl;9c41-dc97/~tND |
fda2-be01/tplx;be55-bf3e/~tNl;0edc-72bf/~tND |
tests/fingerprint/showcases.csv:204 — MixedTraffic |
462d-10c7/tplx;727b-d26a/~tNl;62c4-cbc2/~tND |
3530-5a7d/tplx;4bee-ff14/~tNl;7588-8d92/~tND |
tests/fingerprint/showcases.csv:270 — HCFfragblockack |
41c4-d741/tplx;db22-9b02/~tNl;523f-f640/~tND |
8557-5e23/tplx;fabc-bff1/~tNl;9ab7-ce7d/~tND |
tests/fingerprint/showcases.csv:344 — txop/General |
9fdc-f33e/tplx;5ee8-bcc1/~tNl |
0a69-2e4d/tplx;fe18-b226/~tNl |
tests/fingerprint/examples.csv:9 — adhoc/qos MacQos -r 1 |
ed2f-2d62/tplx;8a06-c4b3/~tNl;cddb-a568/~tND |
3d9e-813a/tplx;e30d-288b/~tNl;b8e0-ee55/~tND |
The failures were consistent with the intentional trajectory-changing parts of the branch:
- Block Ack transaction state, response handling, and final-fragment processing changed in
OriginatorBlockAckAgreementHandlerandHcf, directly affecting the Block Ack examples and showcases. - QoS pending-frame eligibility and A-MSDU selection/extraction changed in
OriginatorQosMacDataServiceandBasicMsduAggregationPolicy, affecting QoS queue selection and event/packet trajectories. - TXOP continuation changed in
HcfFs, accounting for theshowcases/wireless/txoptrajectory change. MacQosWithBlockAcknow receivessameTransmissionStartTimeCheck = "ignore"directly in its configuration, changing that configuration's recorded trajectory.- Both
adhoc/qosrepetitions exercise the revised QoS/HCF path through the configuration's${false, true}Block Ack variation; only repetition 1 enables that variation and mismatched, while repetition 0 retained its prior expectation.
These mechanisms explain why the affected fingerprints changed and why the mismatches were confined to the related QoS, Block Ack, fragmentation, and TXOP rows. The earlier assessment did not claim that the first event-level divergence had been independently localized for every one of the eight rows.
The remaining full-suite failure was unrelated to the branch: the existing tests/fingerprint/ethernet.csv row with $datarate == 5Gbps && $duplex == false failed during initialization because EthernetCsmaMacPhy rejects 5 Gbps half-duplex Ethernet. It produced no replacement fingerprint.
After assessing the eight trajectory changes, their expected values were updated in tests/fingerprint/examples.csv and tests/fingerprint/showcases.csv by commit e02c768a4b. No expectation was added for the unrelated Ethernet initialization failure.
Post-update focused confirmation
The same eight changed rows were then rerun at e02c768a4b:
./fingerprinttest -d \
-m '/examples/adhoc/qos/ .*MacQos -r 1' \
-m '/examples/wireless/qos/ .*MacQosWith(BlockAck|TransactionalBlockAck)' \
-m '/showcases/wireless/blockack/' \
-m '/showcases/wireless/fragmentation/ .*HCFfragblockack' \
-m '/showcases/wireless/txop/' \
-f 'tplx' -f '~tNl' -f '~tND' \
-l fingerprinttest-final-comments-20260822.outResult at e02c768a4b: exit status 0; eight tests selected; eight passed; no mismatches. The saved artifact is tests/fingerprint/fingerprinttest-final-comments-20260822.out. The wrapper used debug runners via -d; no release artifact was executed.
Together, the two runs establish the sequence explicitly: the full suite exposed eight related stale expectations; those eight expectations were updated; and the exact affected rows subsequently passed against the updated baselines.
No new unit test was launched for this report. Existing unit_tests.log records the explicitly filtered debug Ieee80211AddbaTransaction_1.test passing on 2026-08-22. It covers the existing transaction ordering, ACK/final-fragment, stale active-sequence, local-veto, queue compatibility, shared-buffer, and reorder-reset behavior, but not deliberate pending-state reentrant cancellation or a deliver-complete teardown alternative.
The IEEE corpus status was fresh for IEEE Std 802.11-2024 (80211ax-2024); PDF inspection was not needed.
Architectural review
Applicable requirements include R-SCOPE-WIRELESS, R-RUN-REPRO, R-DIST-COMPAT, AR-ORG-CONTRACTS, AR-QUEUE-ROLES, AR-QUAL-LOGGING, AR-QUAL-TESTS, AR-QUAL-DETERMINISM, AR-WLAN-STD-TRACE, AR-WLAN-ARCH-OWNERSHIP, AR-WLAN-MAC-EXCHANGE, AR-WLAN-MAC-SEQUENCE, AR-WLAN-MAC-QOS, AR-WLAN-OBS-EVENTS, and AR-WLAN-QUAL-TESTS.
No new AV-*, AS-*, NV-*, or NS-* disposition is needed. The comments do not require a source or sealing change.
General semantic checklist
- PASS —
AR-ORG-VIS-SPLIT— No protocol visualization logic was introduced. - PASS —
AR-ORG-KERNEL— The changes consume OMNeT++ lifecycle and queue facilities without reimplementing them. - PASS —
AR-MOD-COMPOSITION— Transaction, agreement, queue, aggregation, and reorder responsibilities remain separated. - PASS —
AR-COM-SOCKETS— No application/transport socket interaction is involved. - PASS —
AR-COM-DIRECT— No zero-time message substitutes for direct intra-node coordination. - PASS —
AR-OBS-NED-TRUTH— NED remains authoritative for parameters, signals, and statistics. - PASS —
AR-OBS-INTROSPECTION— No on-air protocol lacks introspection support. - PASS —
AR-CFG-INFER / DRY— Transaction identity and agreement state remain centralized. - PASS —
AR-CFG-PARAMS— No malformed parameter contract is introduced. - PASS —
AR-EXT-NOCORE— No protocol addition modifies core registration machinery. - PASS —
AR-BUILD-DECLARATIVE— No machine-specific build value was added. - PASS —
AR-QUAL-NAMING— New semantic names follow the conventions. - PASS —
AR-QUAL-LOGGING— Provider and invariant violations throw instead of merely logging. - PASS —
AR-QUAL-TESTS— Focused correctness tests accompany the changed contracts; the two proposed hardening/design alternatives need tests only if implemented. - PASS —
AR-QUAL-DISPLAY— No new module type lacks a distinguishing display declaration.
REVIEW: 15 PASS, 0 FLAG, 0 QUESTION
IEEE 802.11 semantic checklist
- PASS —
AR-WLAN-STD-TRACE— Setup, SSN, veto, retry, reorder, and teardown decisions are traceable to IEEE Std 802.11-2024; teardown draining is correctly identified as unspecified. - PASS —
AR-WLAN-STD-GATING— Block Ack behavior remains gated by configured support, policy, and agreement state. - PASS —
AR-WLAN-ARCH-BOUNDARIES— MAC, agreement policy, handler, queue, aggregation, and data-service responsibilities remain separated. - PASS —
AR-WLAN-ARCH-OWNERSHIP— Agreement, transaction, sequence, retry, queue, and reorder state each retain one owner. - PASS —
AR-WLAN-ARCH-VARIANTS— Negotiation decisions remain replaceable policies. - PASS —
AR-WLAN-FRAME-REPRESENTATION— ADDBA/DELBA remain typed on-air chunks; transaction identity remains local metadata. - PASS —
AR-WLAN-PHY-AUTHORITY— No PHY mode calculation is duplicated in MAC code. - PASS —
AR-WLAN-PHY-TIMING— No PHY or interframe timing constant is duplicated. - PASS —
AR-WLAN-MAC-EXCHANGE— Setup, response, timeout, retry, and teardown retain explicit owners. - PASS —
AR-WLAN-MAC-SEQUENCE— SSN and reorder-window paths use centralized cyclic sequence handling. - PASS —
AR-WLAN-MAC-QOS— TID/agreement lookup and per-AC queue state remain centralized. - PASS —
AR-WLAN-MAC-MULTIUSER— No MU scheduling or PPDU construction is changed. - PASS —
AR-WLAN-OBS-EVENTS— Agreement lifecycle and packet-drop events are emitted by their owners. - QUESTION —
AR-WLAN-QUAL-TESTS— Confirm the intended model scope for custom pre-agreementNO_ACKtraffic and the desired handling of complete frames stranded at recipient reorder teardown; add focused tests if either policy changes.
WLAN REVIEW: 13 PASS, 0 FLAG, 1 QUESTION
Final merge recommendation
The twelve final-review comments do not establish a current merge blocker. The directly related updated fingerprints pass in debug mode at e02c768a4b. Prefer hardening reentrant cancellation in this PR if the callback API is expected to tolerate arbitrary caller ordering; otherwise document the invalidate-before-cancel invariant. Treat recipient reorder draining as an explicit model-policy decision, because the loss is real but the claimed IEEE teardown mandate is not.
Avoid building a complete snapshot of the pending queue for every successful A-MSDU aggregation. Validate each policy-selected subframe through the packet extractor's identity lookup instead, while retaining duplicate detection, eligibility checks, pre-mutation validation, and existing RAII ownership handling. Add a focused regression with two selected frames and a 128-packet trailing backlog. Verify that aggregation removes only the selected frames and performs no packet-collection enumeration. Debug validation: - make MODE=debug -j$(nproc) - inet_run_unit_tests -m debug -f 'Ieee80211AddbaTransaction_1\.test' - MacQos runs 0 and 1 to 1.1 s: approximately 5x faster - focused fingerprints: run 0 unchanged; run 1 completes without the former CPU timeout and has a reproducible pre-existing ADDBA trajectory mismatch unrelated to this optimization
The latest full debug run used only `tplx`, `~tNl`, and `~tND` and selected 1,753 rows: 1,744 verified, eight mismatched, and one with an initialization error. The eight mismatches are:
| Test row | Recorded | Current calculated |
|---|---|---|
| `tests/fingerprint/examples.csv:661` — `MacQosWithBlockAck` | `8306-3cd3/tplx;26d1-9165/~tNl;e0fc-7553/~tND` | `040e-6428/tplx;557f-bcad/~tNl;68e6-5a6d/~tND` |
| `tests/fingerprint/examples.csv:662` — `MacQosWithTransactionalBlockAck` | `c4ff-d71f/tplx;68ba-2827/~tNl;9a98-cc02/~tND` | `b608-4cad/tplx;6f80-132d/~tNl;9b78-3d6c/~tND` |
| `tests/fingerprint/showcases.csv:202` — `NoFragmentation` | `aa2d-5d35/tplx;2094-1f2a/~tNl;1470-1e1b/~tND` | `7ecf-66f5/tplx;193b-2155/~tNl;fa4f-92fc/~tND` |
| `tests/fingerprint/showcases.csv:203` — `Fragmentation` | `7ae9-e07d/tplx;db8b-3b81/~tNl;9c41-dc97/~tND` | `fda2-be01/tplx;be55-bf3e/~tNl;0edc-72bf/~tND` |
| `tests/fingerprint/showcases.csv:204` — `MixedTraffic` | `462d-10c7/tplx;727b-d26a/~tNl;62c4-cbc2/~tND` | `3530-5a7d/tplx;4bee-ff14/~tNl;7588-8d92/~tND` |
| `tests/fingerprint/showcases.csv:270` — `HCFfragblockack` | `41c4-d741/tplx;db22-9b02/~tNl;523f-f640/~tND` | `8557-5e23/tplx;fabc-bff1/~tNl;9ab7-ce7d/~tND` |
| `tests/fingerprint/showcases.csv:344` — `txop/General` | `9fdc-f33e/tplx;5ee8-bcc1/~tNl` | `0a69-2e4d/tplx;fe18-b226/~tNl` |
| `tests/fingerprint/examples.csv:9` — `adhoc/qos MacQos -r 1` | `ed2f-2d62/tplx;8a06-c4b3/~tNl;cddb-a568/~tND` | `3d9e-813a/tplx;e30d-288b/~tNl;b8e0-ee55/~tND` |
The direct branch mechanisms justify investigating these changes: Block Ack transaction state and final-fragment handling changed in `OriginatorBlockAckAgreementHandler` and `Hcf`; QoS queue eligibility and A-MSDU extraction changed in `OriginatorQosMacDataService` and `BasicMsduAggregationPolicy`; TXOP continuation changed in `HcfFs`; and `MacQosWithBlockAck` now receives `sameTransmissionStartTimeCheck = "ignore"` directly in its configuration. Both adhoc repetitions exercise the revised QoS/HCF path with the configuration's `${false, true}` Block Ack variation, but only repetition 1 mismatches in the latest run. These mechanisms explain why trajectories can change, but the first event-level divergence has not yet been localized for every failing row.
|
Devin's response to that report: The report's key claim verifies. All eight fingerprint rows the report says were updated in
Two details worth noting, both consistent with the report:
Overall assessment of the final reportThe prior merge blocker is genuinely closed. The stale expectations the previous report flagged are now updated in the diff, and the "after" values match the recomputed fingerprints. What I can independently confirm is that the CSVs were edited to the claimed values; what I cannot verify from the diff is the report's assertion that the focused rerun at The dispositions are well-calibrated. The two open items are correctly classified as non-blocking:
Points I'm taking on trust (not verifiable from the diff):
Bottom line: The report's conclusion — no current merge blocker, two non-blocking follow-ups (harden reentrant cancellation; decide teardown drain policy explicitly) — is well-supported. The fingerprint evidence, which was the only hard gate, checks out against the actual diff. The remaining |
1 participant
Summary
MacQosWithTransactionalBlockAckexample/fingerprint exercising successful and timed-out transactionsWhy
The previous implementation did not correlate ADDBA responses with an outstanding dialog token, treated policy acceptance as sufficient regardless of status, did not implement response-timeout recovery, and created recipient agreement state before the successful response was transmitted. Meanwhile, queued frames could receive sequence numbers while setup was pending, so the first frame sent under an accepted agreement could diverge from the SSN carried by the request.
This change follows IEEE Std 802.11-2024 clauses 9.6.4.2, 10.25.2, 10.25.6.6.1, 11.5.2.2, and 11.5.2.3. It is based directly on
masterand has no dependency on Compressed Block Ack support.Validation
make MODE=release -j$(nproc)make MODE=debug -j$(nproc)inet_run_unit_tests -m release -f '(Ieee80211AddbaTransaction_1|Ieee80211OnWireBitCompliance_1)\.test'— 2/2 passed./fingerprinttest -d -m '/examples/wireless/qos/.*MacQosWithTransactionalBlockAck' -f 'tplx' -f '~tNl' -f '~tND'— 1/1 passedgit diff --checkThe complete release unit run passed all ADDBA/IEEE 802.11 tests; its 12 unexpected failures were confined to unrelated TCP receive-queue, clock, and oscillator tests. The complete fingerprint run identifies intentional trajectory changes in the pre-existing
MacQosWithBlockAckcase and three wireless Block Ack showcase cases; their expected rows are deliberately not updated in this draft.