Svc/FileDownlink: add Reset command and stall warning to recover from a wedged downlink

[rvaccone]

Related Issue(s)	Fixes #5447
Has Unit Tests (y/n)	y
Documentation Included (y/n)	y
Generative AI was used in this contribution (y/n)	AI

---

Change Description

A downlink whose buffer is never returned wedges FileDownlink in WAIT until reboot.
Cancel can't clear it because it waits on the same return, and Svc::Health can't see it
because pingIn still answers.

Per @bocchino's review, Cancel is unchanged — the protocol break is a new command:

• Reset (0x03) completes the active transfer without waiting on the buffer return, then
  drains the file queue, answering each dropped request rather than discarding it
  (ComQueue.FLUSH_QUEUE pattern, except entries here carry caller context). Emits
  DownlinkReset (0x13) with the drop count.
• Reset reports STATUS_ERROR to port clients for every request it drops, regardless of
  FILEDOWNLINK_COMMAND_FAILURES_DISABLED, so DpCatalog keeps the product for a retry.
  Command responses keep the flag mapping.
• DownlinkStalled (0x14) warns once per wait when a downlink sits too long on a buffer
  return. Optional stallTimeout on configure(), default off, wired through
  FileHandlingConfig. Observation only, no automatic action.
• Dead m_timeout / m_bufferSize / exitFileTransfer removed; SDD §3.5.2 corrected.

Testing/Review Recommendations

15 UTs (6 new), green under ASan/UBSan; all 4 commits build and pass individually.

Two calls worth a look: the drain is bounded by queue depth rather than a live
getMessagesAvailable() count, which isn't serialized against a concurrent enqueue; and
DownlinkStalled is intentionally not throttled, since a count throttle only clears at
transfer start and a slow link could exhaust it, then go silent through the actual wedge.

AI Usage (see policy)

Written with Claude Code and reviewed by me.

IAMAI

[bocchino]

An operator's Cancel should be a reliable escape hatch and must not depend on the very buffer return that is already wedged.

I'm not sure I agree with this reasoning. The existing CANCEL behavior follows the general flow-control protocol of file downlink (wait until the last packet comes back before allowing a new one to be sent). The new proposed behavior breaks the protocol. While breaking the protocol may be needed in some off-nominal cases, it seems dangerous to automatically break the protocol in every case that a transmission is canceled. In most cases, the operator probably just wants to end the transmission prematurely, not try to recover from a system fault. Also, it seems the new behavior defaults to masking memory leaks. A true memory leak (unrecoverable dropped packet) seems like a system design flaw that should not be silently ignored.

Have you considered the following undesirable behaviors that the new design may introduce?

1. Operator sends a protocol-breaking CANCEL and then immediately starts a new downlink. After the downlink starts, the packet from the canceled transmission comes back.
2. Operator sends CANCEL and due to a downstream dropped packet, there is a memory leak. This happens over and over, silently, until memory resources are exhausted.

Have you considered keeping the CANCEL behavior as is, and adding a new command RESET to break the protocol by explicit command, in cases where there is no other way to recover from a memory leak?

[rvaccone]

Agreed. Your concern #1 is stronger than stated. Even a slow but healthy downstream can break: a nominal Cancel plus a new downlink rewrites m_memoryStore[FILE_PACKET] if the downstream still references it. The stale ID check protects the state machine, not the memory. On #2, the buffers wrap memory internal to the component, so nothing accumulates. The real cost is hiding the fault, and that point stands.

Rework plan (converting to draft meanwhile):

• Cancel reverts to current behavior.
• New RESET command completes the active transfer immediately. Docs will state it is for a downstream that has been written off, since reused memory can emit garbage if that downstream revives.
• New warning after a configurable time in WAIT, reusing m_curTimer, which WAIT increments but never reads. Observation only, no automatic action, so it does not reintroduce what Remove Timeout from Svc::FileDownlink #4555 removed. Needed because pingIn responds even when wedged, so Svc::Health cannot see this failure.

For the fan out case in #5447, Svc::BufferRepeater with a BufferManager prevents this wedge at the topology level (copies per channel, original returned immediately, strands visible in CurrBuffs). RESET is defense in depth for single chain topologies.

Question: after RESET the component dequeues the next file and likely wedges again. Leave the queue untouched, or drain it with error responses (similar to ComQueue.FLUSH_QUEUE)?

---

_{Disclosure per AI_POLICY.md: AI assisted (Claude Code). Analysis reviewed against devel before posting.}

[bocchino]

Question: after RESET the component dequeues the next file and likely wedges again. Leave the queue untouched, or drain it with error responses (similar to ComQueue.FLUSH_QUEUE)?

It seems reasonable to me to follow the same pattern as the ComQueue. @LeStarch do you have thoughts here?

[rvaccone]

v2 is up. Cancel is unchanged; Reset force-completes the transfer and then drains the queue using the ComQueue pattern, except entries here carry caller context, so each dropped request gets a response rather than being discarded.

Worth a check: the drain is bounded by the configured queue depth, not a live getMessagesAvailable() count, which isn't serialized against a concurrent SendFile_handler enqueue. The trade-off is that a client resubmitting from its completion callback can be drained too.

_{Disclosure per AI_POLICY.md: AI assisted (Claude Code). Reviewed against devel before posting.}

[sylvesterkaczmarek]

One concern with the new Reset drain: with the default FILEDOWNLINK_COMMAND_FAILURES_DISABLED = true, requests that were never transmitted are reported to port clients as STATUS_OK. As the SDD notes, DpCatalog can then mark the product transmitted and remove it. Could Reset's synthetic drop responses always report STATUS_ERROR to port clients, while preserving the legacy command-response mapping if needed?

[rvaccone]

Agreed. The flag's own comment scopes it to keeping a sequence from aborting, so it belongs on the command path only.

Pushed a commit that does this:

• Reset now reports STATUS_ERROR to port clients for every request it abandons, the queued ones and the active one it force-completes. Command responses keep the flag mapping.
• One helper holds the rule for both paths, with UT and SDD updated.

Two things I left alone, called out in the SDD: Cancel still reports STATUS_OK to port clients, and the pre-existing failure paths (open error, zero size, bad offset, read error) still map to OK under the flag. Making those honest would let a missing or unreadable product wedge DpCatalog's catalog transmit, since it stops on any error and retries the same entry first. That needs a DpCatalog-side policy before FileDownlink can change, so I'd handle it separately. Happy to align Cancel here if you'd prefer.

[sylvesterkaczmarek]

Re-reviewed the Reset response semantics. Active and queued port-originated requests now always receive STATUS_ERROR through abortStatus(), independent of FILEDOWNLINK_COMMAND_FAILURES_DISABLED, while command-originated responses retain the legacy flag mapping. The reset-drain tests pin both the active and queued port cases. This resolves the DpCatalog delivery-status issue I raised.

[rvaccone]

Thanks for the re-review.

Rebased onto devel to clear the #5833 conflict (two SDD sandbox paragraphs, no code changes). UTs and Ref build pass locally.

@bocchino this still needs workflow approval, and your call on whether Cancel should also report STATUS_ERROR to port clients.

[sylvesterkaczmarek]

Rechecked the rebased head. The Reset response implementation and the active/queued port-client regressions relevant to my earlier review are unchanged from the approved head, so the STATUS_ERROR semantics remain intact. The rebase also brings unrelated sandbox/subtopology changes from devel, but they do not alter this Reset path. My approval stands.

[LeStarch]

We appreciate you working on this. We have reconsidered this approach and agreed that the RESET approach needs discussion with the CCB. The original issue was to work on the CANCEL command, it was correct to identify that the RESET approach was much better. However, there were many issues we realized with the RESET approach (i.e. memory leaks, ownership leaks, corruption, etc). Given these potential problems, we would like to reconsider the RESET approach.

To that end we have submitted: #5864 for that exact purpose. In that issue we have recorded the original CANCEL request, and we have recorded this implementation, which will influence the discussion.

For now I will close this PR, but it is tracked in the new ticket, so it can be revived once the CCB has a strong recommendation for reset.

[rvaccone]

Thanks for taking the time to explain, and for capturing the history in #5864. That all makes sense.

If it's useful, the stall warning and the dead-member cleanup don't depend on Reset. Happy to split those into a small separate PR. Either way, I'll follow #5864 and can pick this back up once the CCB decides.