[durasi]

Change Description

Adds a GenerateDp command to Svc::FileManager that packages a file into data products.

Following the design described in #5515, the command takes a fileName and a chunkSize, and each chunk is emitted as a pair of records:

• FileChunkHeaderRecord: a struct carrying the source file name, the offset of the chunk within the file, and the number of data bytes
• FileChunkDataRecord: a U8 array record with the chunk bytes

Ground tools can reassemble the original file from the chunks, and the command works regardless of the size of the buffers allocated to data products.

Implementation notes:

• Chunks are paced by the rate group using the same internal port pattern as ListDirectory, one chunk per tick by default (FileManagerConfig::CHUNKS_PER_RATE_TICK), so per tick work stays bounded
• The command response is deferred until the last chunk is sent, matching the ListDirectory pattern
• The requested chunk size is clamped to FileManagerConfig::GENERATE_DP_MAX_CHUNK_SIZE, which bounds the component member read buffer so no dynamic allocation is needed
• A pair of records per chunk was chosen over a single fixed size struct record because FPP struct members are fixed size; with array records the serialized size tracks the real chunk size instead of always paying for the maximum

Rationale

Closes #5515.

Testing/Review Recommendations

Run fprime-util check in Svc/FileManager. 21 tests pass locally on macOS (AppleClang 21, ASan and UBSan enabled), including four new cases: nominal chunking of a 100 byte file into 4 chunks with a 32 byte chunk size, a missing file, an empty file, and a chunk size larger than the configured maximum.

Note: runRateGroupCycles in the tester previously stopped cycling as soon as directory listing was idle, so it was generalised to keep cycling while either directory listing or data product generation is active.

[durasi]

Added the SDD section for the command.

Two notes on scope:

The data product ports are left unconnected in Svc/Subtopologies/FileHandling, matching how fileManager.schedIn is exposed for the deployment to wire up. The command checks isConnected and fails with an event if the ports are not connected, so nothing breaks when they are absent. Happy to wire them into the FileHandling subtopology if you would prefer that here.

On the ground side, reassembling the chunks into the original file would need a small tool in fprime-gds that reads the FileChunkHeaderRecord and FileChunkDataRecord pairs and writes them out by offset. That felt like a separate change, but I am glad to open an issue for it or take it on if useful.

Full build (fprime-util build --all, 763 targets) and the FileManager unit tests (21 passing) are clean locally.

[durasi]

Added beginOffset and endOffset per the discussion in #5515.

Semantics as described there: an endOffset of 0 means the end of the file, so GenerateDp(file, chunkSize, 0, 0) still packages the whole file; an endOffset past the end of the file is clamped to the file size; and a beginOffset at or past the endOffset is rejected with a GenerateDpInvalidRange event and a validation error response. Since each chunk header carries the absolute offset within the source file, chunks produced by separate commands reassemble on the ground without extra bookkeeping.

Two more tests, so 23 passing locally: bytes [20, 60) of a 100 byte file with a 16 byte chunk size produces 3 chunks, and an inverted range is rejected. Full build is clean. SDD updated.

[durasi]

Applied the sequence-friendly failure handling from #5515: every failure path now emits its warning event and still responds with OK, so a bad file name or a transient resource problem will not halt a sequence that contains the command. That covers file open and read errors, the size query, an invalid offset range, container allocation, serialization, and a request arriving while another is in progress. The events are unchanged, so operators still see exactly what went wrong.

23 tests still passing locally and the full build is clean. SDD notes the behaviour.

[github-actions]

Coverage report — base devel

Overall (line): 83.66% → 83.69% (+0.03)
Regression threshold: 0.50% (line).

Regressions

(none over threshold)

Modules changed

Module	Line	Δ	Function	Δ	Branch	Δ
Svc/FpySequencer	86.31	-0.03	98.59	+0.00	77.18	+0.00
Svc/FileManager	88.03	+0.01	94.44	+1.11	78.99	-1.30
Svc/CmdDispatcher	97.00	+0.03	91.67	+0.00	91.49	-0.09
Os/Generic/Types	92.14	+0.35	92.86	+0.00	76.64	+1.87
Os/Posix	64.56	+0.40	85.26	+0.00	45.94	+0.46
Fw/DataStructures	98.31	+0.53	97.14	+0.00	83.15	+0.92

Modules without UTs

CFDP/Checksum/GTest, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports/DataTypes, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Obj, Fw/Port, Fw/Prm, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Ccsds/Types, Svc/FatalHandler, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComCcsdsSdls, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/DpCompression, Svc/Subtopologies/FileHandling, Svc/Subtopologies/FileHandlingCfdp, Svc/Subtopologies/FileHandlingCfdp/FileHandlingCfdpConfig, TestDeploymentsProject/Ref/DpDemo, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestConfigDeployment, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestLinkDepends, cmake/test/data/TestDeployment/TestRelative, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent

[durasi]

Addressed the coverage regression flagged by the bot.

Added three tests covering the failure paths that were not exercised: a container allocation failure, a request arriving while another generation is in progress, and a serialization failure (the tester can now hand out an undersized buffer for that one).

I also removed two guards in processDpChunks that turned out to be unreachable: the loop returns as soon as the requested range is exhausted, so neither the idle-state check at the top nor the zero-remaining check could ever be hit.

Svc/FileManager line coverage is now 88.4% locally, up from 84.4% and above the 88.02% baseline. 26 tests passing.

On the failing YAMCS Reference job: it looks unrelated to this change, the log shows "YAMCS failed to start" before any test runs, and the same job is failing on other PRs against devel. Happy to look into it if that is not the case.

[timcanham]

Good work! One moderate and a few small changes requested.

[timcanham]

Add a priority argument to this function or projects will be stuck with 10, which is pretty high for a file. It could starve more important items. The convention is that a priority of zero reverts to the default, while > 0 is the new priority.

[timcanham]

Move default priority to a config FPP file so it can be adjusted by projects.

[timcanham]

This may be answered later in the code, but it should reflect the size written if a portion of a file was written instead of the whole file.

[timcanham]

This is used in a number of places, so it might be difficult to disambiguate. If you want to reuse an event, a common practice is to add an enum argument so the location of the error can be uniquely found.

[timcanham]

Using the rate group to write chunks could be very slow (a small piece per cycle) and slow down the rate group too much. It's not a bad idea if projects want to mete out the file, but others could want it to go quickly for power reasons, etc. Let's make a "mode" option where it could use the rate group OR do it all at once.

[timcanham]

If the file size math was right, readSize should always come back as the requested size, so I would check for that instead of readSize == 0. You can stash the computed size in another variable and check it against readSize after the read.

[timcanham]

@durasi Did you try running this and decoding with the ground tools?

[durasi]

Thanks for the review. All six points are addressed in the latest commit, and the branch is rebased on current devel.

Priority argument. The command now takes a priority, with zero reverting to the configured default and a non-zero value applying to the containers of that request.

Default priority in config. Moved to a new default/config/FileManagerCfg.fpp as Svc::FileManagerCfg::DEFAULT_DP_PRIORITY.

Mode option. Added a GenerateDpMode enum with PACED and IMMEDIATE. In paced mode the rate group meters the chunks out as before; in immediate mode the whole range is written in the command handler, so a project that wants the file out quickly is not limited by the rate group.

GenerateDpStarted size. Now reports the number of bytes that will be written, so a partial range is reflected correctly rather than the size of the whole file.

Failure disambiguation. Added a GenerateDpStage enum argument to GenerateDpFailed covering OPEN, SIZE, SEEK, READ, SERIALIZE and BUSY.

Read size check. The requested size is now stashed and compared against the returned size, instead of only checking for zero.

Two more tests, so 28 passing locally, and line coverage is 88.1%. The priority test recovers the value from the serialized container header, so it checks what actually goes out rather than just the member.

On decoding with the ground tools: no, I have only exercised this through unit tests so far. Svc/FileManager does not have its data product ports wired in the FileHandling subtopology, so a real run needs that wiring first, and I did not want to add it without asking. If you would like the end-to-end check, I am happy to wire the ports through the subtopology, run Ref, generate a data product from a file and decode it with fprime-gds data-products decode, and report back.

[lestarch-autobot]

[Architecture] suggestion IMMEDIATE mode performs unbounded work in an async command handler, departing from this component's bounded-per-tick pacing pattern.

The baseline FileManager (active, all-async commands, sync schedIn delegating via the internal run port) deliberately bounds per-dispatch work (FILES_PER_RATE_TICK, CHUNKS_PER_RATE_TICK). processDpChunks(0) in the handler loops over the whole range, blocking the queue (other commands, pingIn health pings) for a duration proportional to file size. The PR description documents pacing but not this departure; see "Cyclic Notification Pattern" in docs/user-manual/framework/component-and-port-selection.md. Please add the rationale to the PR description and warn operators in the model doc.

Suggested change

	@ Emit all chunks in the command handler, completing immediately
	IMMEDIATE = 1
	@ Emit all chunks in the command handler, completing immediately; runtime is
	@ unbounded by range size and blocks the component queue (commands, pings)
	IMMEDIATE = 1

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Architecture] future work DP pacing now depends on the preexisting run drop internal-port latch, which can stall permanently if the queue is ever full.

schedIn_handler sets m_runQueued true before run_internalInterfaceInvoke(); the internal port is declared drop, so on a full queue the message is discarded and m_runQueued never resets, silencing all future ticks. Preexisting (lines unchanged by this PR), but a stall now also strands an in-progress GenerateDp: m_dpFile stays open and the deferred command response is never sent.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Maintainability] could fix maint-unclear-naming: GenerateDpBufferFailed fired for unconnected ports.

The event's format string says "Failed to allocate data product buffer", but this path fires when the DP ports are not connected — an operator debugging from the event log will chase a buffer-pool problem instead of a topology problem. A dedicated GenerateDpStage::NOT_CONNECTED value on the existing GenerateDpFailed event would name the actual cause.

[lestarch-autobot]

[Maintainability] could fix maint-code-duplication: the close + log + respond + return exit block is repeated on four setup-failure paths (OPEN, SIZE, bad range, SEEK), three of which must remember m_dpFile.close().

A fix to this exit sequence must be applied in four places, and the next guard clause added between open() and the state hand-off will silently leak the file handle. A small helper, e.g. failDpSetup(logFileName, stage, status) that closes, logs, and responds, makes the cleanup structural.

[lestarch-autobot]

[Maintainability] could fix maint-dead-code: m_dpFileSize is written (ctor, GenerateDp_cmdHandler, finishDpGeneration) but never read.

It is a private member of a final class, so nothing outside this component can reach it; the range logic already uses the local fileSize. Dead state invites future readers to assume it is load-bearing. If removed, also drop the ctor initializer and the two assignments in FileManager.cpp.

Suggested change

	//! Total size of the file being packaged
	FwSizeType m_dpFileSize;

[lestarch-autobot]

[Maintainability] could fix maint-unclear-parameters: chunkLimit == 0 is a sentinel meaning "emit everything".

At the call site processDpChunks(0) reads as "process zero chunks" — the opposite of what happens — and a future caller passing a computed limit that can reach 0 flips pacing into a full drain silently. Passing the GenerateDpMode (or a named constant such as ALL_REMAINING_CHUNKS) makes the meaning visible at the call site.

[lestarch-autobot]

[Maintainability] suggestion maint-inconsistent-local-convention: missing blank line between CalculateCrcSucceeded and the new GenerateDpStarted event.

Every other event in this file is separated by a blank line; the missing separator makes the new block read as part of the previous event when scanning.

Suggested change

	format "{} has CRC value 0x{x}"
	format "{} has CRC value 0x{x}"

[lestarch-autobot]

[Maintainability] could fix maint-dead-code: m_dpBytesSent is accumulated in productSend_handler but never asserted or read.

It reads as if a byte-count check exists somewhere, so a future test author will trust coverage that is not there. Either assert it in the chunking tests (it would strengthen generateDpSucceed) or remove the member and its two writes in FileManagerTester.cpp.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Documentation] could fix docs/reference/system-functional/file-management.md does not list the new GenerateDp capability.

The "File System Operations" bullet list in that reference catalogs the File Manager's ground commands; this PR adds data-product generation without a corresponding entry. (Anchored here because the reference file is not in the PR diff.) Suggested addition: - Package a file into data products (GenerateDp).

[lestarch-autobot]

[Documentation] could fix docs/reference/system-functional/subtopology-file-handling.md capability list omits the new command.

That reference describes File Manager as "(list, remove, create directory, move, copy, concatenate, hash)"; this PR adds data-product generation, so the catalog line is now stale. (Anchored here because the reference file is not in the PR diff.)

[lestarch-autobot]

[Documentation] suggestion The SDD does not document the busy-rejection behavior of GenerateDp.

GenerateDp_cmdHandler rejects a request while another is in progress (GenerateDpFailed at stage BUSY, response still OK). Operators sequencing multiple GenerateDp commands need this in the SDD.

Suggested change

	Failures during data product generation emit a warning event but still return a
	successful command response. A bad file name or a transient resource problem
	therefore does not stop a command sequence that happens to contain the command,
	while operators still see exactly what went wrong in the event log.
	Failures during data product generation emit a warning event but still return a
	successful command response. A bad file name or a transient resource problem
	therefore does not stop a command sequence that happens to contain the command,
	while operators still see exactly what went wrong in the event log. Only one
	request runs at a time: a `GenerateDp` command received while another is in
	progress is rejected with a `GenerateDpFailed` event at the `BUSY` stage.

[lestarch-autobot]

[Documentation] suggestion The SDD does not say that a chunk size of zero selects the configured maximum.

The handler maps chunkSize == 0 to GENERATE_DP_MAX_CHUNK_SIZE, but the SDD only describes the clamp for oversized values.

Suggested change

	that request only. The requested chunk size is clamped to
	`FileManagerConfig::GENERATE_DP_MAX_CHUNK_SIZE`, which bounds the read buffer
	held by the component.
	that request only. The requested chunk size is clamped to
	`FileManagerConfig::GENERATE_DP_MAX_CHUNK_SIZE`, which bounds the read buffer
	held by the component; a chunk size of zero also selects this maximum.

[lestarch-autobot]

[Documentation] suggestion The SDD documents only endOffset == 0 as "end of file", but the handler also clamps endOffset > fileSize to the file size.

Suggested change

	time. An end offset of zero means the end of the
	file, so a begin and end offset of zero packages the whole file. Ranges let an
	time. An end offset of zero, or one beyond the end of the file, means the end of the
	file, so a begin and end offset of zero packages the whole file. Ranges let an

[lestarch-autobot]

[Documentation] could fix The chunk-record format is a ground contract, but no ground-side (fprime-gds / GDS user-doc) reassembly documentation or tooling reference exists yet.

The SDD states "Ground tools reassemble the original file from these records"; the external doc gap (fprime-gds docs) should be tracked so the reassembly contract (header record followed by data record, absolute offsets) is documented where ground operators look.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Security] must fix IMMEDIATE mode performs ground-controlled unbounded work in the command handler (ground-overflow-or-ddos).

A single GenerateDp with mode = IMMEDIATE on an arbitrarily large file reads and emits the whole range inside the async command handler. This active component's queue (subsequent commands, pingIn) is blocked for a duration proportional to the ground-controlled file size — on deployments where Svc::Health pings FileManager, a large file can starve ping responses and trigger a FATAL, a ground-reachable DoS. Each iteration also requests a DP container, so one command can drain the DP buffer pool in a burst. Bound the per-call work (e.g. cap the chunks emitted per invocation via a config constant) or remove IMMEDIATE mode.

Suggested change

	this->processDpChunks(0);
	if (mode == FileManager_GenerateDpMode::IMMEDIATE) {
	this->processDpChunks(FileManagerConfig::GENERATE_DP_MAX_IMMEDIATE_CHUNKS);
	}

(best-effort fix; verify before applying — requires adding GENERATE_DP_MAX_IMMEDIATE_CHUNKS to FileManagerConfig.hpp)

cc @LeStarch @thomas-bc @bitWarrior — low-confidence finding, please confirm.

[lestarch-autobot]

[Test Quality] must fix GenerateDpStarted is asserted by count only; its payload (fileName, bytesToWrite) is never checked.

A new event with declared payload needs a payload assertion — the test passes even if the component reports the wrong file name or byte count (100 bytes here).

Suggested change

	ASSERT_EVENTS_GenerateDpStarted_SIZE(1);
	ASSERT_EVENTS_GenerateDpStarted_SIZE(1);
	ASSERT_EVENTS_GenerateDpStarted(0, fileName, 100);

[lestarch-autobot]

[Test Quality] must fix GenerateDpFailed is asserted by count only; the stage/status payload is never checked.

Without asserting stage == OPEN, this test would pass if the failure occurred at any other stage for any other reason. (best-effort fix; verify the expected status value before applying)

Suggested change

	ASSERT_EVENTS_GenerateDpFailed_SIZE(1);
	ASSERT_EVENTS_GenerateDpFailed_SIZE(1);
	ASSERT_EVENTS_GenerateDpFailed(0, "no_such_dp_file", FileManager_GenerateDpStage::OPEN, static_cast<U32>(Os::File::DOESNT_EXIST));

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Test Quality] must fix GenerateDpInvalidRange is asserted by count only; the offset/size payload is never checked.

The event carries beginOffset, endOffset, and fileSize; asserting them verifies the rejection was for the requested [60, 20) range on the 100-byte file.

Suggested change

	ASSERT_EVENTS_GenerateDpInvalidRange_SIZE(1);
	ASSERT_EVENTS_GenerateDpInvalidRange_SIZE(1);
	ASSERT_EVENTS_GenerateDpInvalidRange(0, fileName, 60, 20, 100);

[lestarch-autobot]

[Test Quality] must fix GenerateDpBufferFailed is asserted by count only; the fileName payload is never checked.

One-line payload assertion closes the gap.

Suggested change

	ASSERT_EVENTS_GenerateDpBufferFailed_SIZE(1);
	ASSERT_EVENTS_GenerateDpBufferFailed_SIZE(1);
	ASSERT_EVENTS_GenerateDpBufferFailed(0, fileName);

[lestarch-autobot]

[Test Quality] must fix The busy rejection asserts GenerateDpFailed by count only; stage == BUSY is never checked.

Without the payload assertion, this test also passes if the second command failed for an unrelated reason (e.g., OPEN).

Suggested change

	ASSERT_EVENTS_GenerateDpFailed_SIZE(1);
	ASSERT_EVENTS_GenerateDpFailed_SIZE(1);
	ASSERT_EVENTS_GenerateDpFailed(0, fileName, FileManager_GenerateDpStage::BUSY, 0);

[lestarch-autobot]

[Test Quality] suggestion The final runRateGroupCycles(10) drives the first generation to completion with no assertions afterward.

Asserting the second command response and the completion event verifies the busy rejection did not corrupt the in-progress generation.

Suggested change

	this->runRateGroupCycles(10);
	this->runRateGroupCycles(10);
	ASSERT_CMD_RESPONSE_SIZE(2);
	ASSERT_EVENTS_GenerateDpComplete_SIZE(1);
	ASSERT_EVENTS_GenerateDpComplete(0, fileName, 7);

[lestarch-autobot]

[Test Quality] could fix The SIZE, SEEK, and READ failure stages, and the disconnected-DP-port path, have no test coverage.

OPEN, BUSY, SERIALIZE, and buffer-allocation failures are tested, but a short/failed read (file changed underneath), size/seek failures, and the isConnected_productGetOut guard are not. These need fault injection or an unconnected-port tester variant.

[lestarch-autobot]

[Test Quality] could fix Eleven generateDp* test methods repeat an identical create-file / send / dispatch / cycle / assert / rm template.

Factoring a parameterized helper (fileName, size, chunkSize, offsets, priority, mode, expected chunks/events) would make each case a one-liner and reduce maintenance risk. See the repetitive-structure guidance in the F Prime test docs (RBT or parameterized helpers).

[lestarch-autobot]

[Test Quality] suggestion New SKIP() calls reference a macro with no definition in the repository (gtest provides GTEST_SKIP()).

This branch only compiles on non-Linux/Darwin targets, where it will fail to build. The pattern repeats at the 9 other new generateDp* sites (and pre-exists at two listDirectory* sites). (best-effort fix; verify before applying)

Suggested change

	SKIP(); // Commands not implemented for this OS
	GTEST_SKIP(); // Commands not implemented for this OS

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Test Quality] suggestion The default container priority (priority arg == 0) is exercised but its value is never asserted.

generateDpCustomPriority checks the override (42); no test checks that priority 0 maps to FileManagerCfg.DEFAULT_DP_PRIORITY. (best-effort fix; verify the generated constant name before applying)

Suggested change

	ASSERT_EQ(4u, this->m_dpSendCount);
	ASSERT_EQ(4u, this->m_dpSendCount);
	ASSERT_EQ(static_cast<FwDpPriorityType>(FileManagerCfg::DEFAULT_DP_PRIORITY), this->m_dpLastPriority);

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Correctness] could fix correctness-resource-leak: on serialization failure, the container obtained from dpGet_FileDpContainer is dropped without dpSend, leaking the underlying BufferManager allocation.

Trigger: productGetOut returns a valid buffer smaller than the requested dpSize (the port contract does not guarantee the size). Consequence: the buffer is never returned (Fw.DataProductSync has no return port), so repeated failures monotonically exhaust the buffer pool. Filed as could-fix because stock Svc.DpManager + Svc.BufferManager never hand out an undersized valid buffer, so the path is guarded upstream in standard topologies.

[lestarch-autobot]

[Correctness] could fix correctness-other: this #else branch does not compile on targets other than Linux/Darwin.

SKIP() is not defined anywhere in this test (gtest's macro is GTEST_SKIP()), and fileName is declared only inside the #if block but used after the #endif. The same pattern appears in all 10 new generateDp* tests. Consequence: the UT translation unit fails to compile on any other OS. The preexisting tests avoid this by using FAIL() and keeping declarations outside the conditional; declaring fileName before the #if and using GTEST_SKIP() would fix both.

[lestarch-autobot]

[Correctness] could fix correctness-other: GenerateDp never calls emitTelemetry, so the CommandsExecuted/Errors channels do not reflect this command on any path (success or failure).

Trigger: any GenerateDp execution. Consequence: operator-facing command/error counters are wrong for this command, unlike the other FileManager commands (only CalculateCrc shares this preexisting inconsistency). If intentional, a note in the SDD would help.

[lestarch-autobot]

[Correctness] could fix correctness-predicate: processDpChunks treats a limit of 0 as "emit all remaining chunks", so a project that configures CHUNKS_PER_RATE_TICK = 0 silently gets the whole file emitted in one tick — the opposite of the pacing intent.

Trigger: FileManagerConfig::CHUNKS_PER_RATE_TICK set to 0 (a plausible "pause" configuration). Consequence: unbounded work in a single rate-group tick. A compile-time guard removes the ambiguity (best-effort fix; verify before applying).

Suggested change

	this->processDpChunks(FileManagerConfig::CHUNKS_PER_RATE_TICK);
	static_assert(FileManagerConfig::CHUNKS_PER_RATE_TICK > 0, "DP chunk pacing must be positive");
	this->processDpChunks(FileManagerConfig::CHUNKS_PER_RATE_TICK);

[lestarch-autobot]

[Correctness] future work correctness-state-machine (preexisting): the internal run port is declared drop; if a run message is dropped on a full queue, m_runQueued stays true forever and schedIn_handler's compare-exchange never re-queues, permanently wedging paced operations.

Trigger: component queue full at the moment run_internalInterfaceInvoke is called. Consequence: directory listing — and now paced GenerateDp — stalls forever and the deferred command response is never sent. Preexisting mechanism, but this PR adds a second waited command that depends on it.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[C++ Design] suggestion CPP-31 (silent truncation): Fw::String capacity is independent of FileNameStringSize; the stored name can silently truncate before it is written into FileChunkHeaderRecord.

The command arg is sized FileNameStringSize (240) but Fw::String is sized FW_FIXED_LENGTH_STRING_SIZE. Both are project-configurable; a project that shrinks the latter below the former silently truncates the file name that ground tools use to reassemble chunks. Fw::FileNameString (Fw/Types/FileNameString.hpp) is sized exactly to FileNameStringSize, making truncation impossible.

Suggested change

	Fw::String m_dpFileName;
	Fw::FileNameString m_dpFileName;

[lestarch-autobot]

[C++ Design] must fix CPP-19 (initialize all variables): the new data-product bookkeeping members are uninitialized until resetDpState() runs.

The tester constructor does not initialize these members and does not call resetDpState(); any future test (or handler call) that touches them before resetDpState() reads indeterminate values. Default member initializers make the fixture safe by construction.

Suggested change

	U8 m_dpContainerData[FW_COM_BUFFER_MAX_SIZE];
	//! Buffer wrapping m_dpContainerData
	Fw::Buffer m_dpContainerBuffer;
	//! Number of containers sent by the component
	U32 m_dpSendCount;
	//! Total number of data bytes observed across sent containers
	FwSizeType m_dpBytesSent;
	//! Whether the get port should fail, for testing the failure path
	bool m_dpGetShouldFail;
	//! Whether the get port should hand out a buffer too small for a chunk
	bool m_dpGetUndersizedBuffer;
	//! Priority carried by the most recently sent container
	FwDpPriorityType m_dpLastPriority;
	U8 m_dpContainerData[FW_COM_BUFFER_MAX_SIZE] = {};
	//! Buffer wrapping m_dpContainerData
	Fw::Buffer m_dpContainerBuffer;
	//! Number of containers sent by the component
	U32 m_dpSendCount = 0;
	//! Total number of data bytes observed across sent containers
	FwSizeType m_dpBytesSent = 0;
	//! Whether the get port should fail, for testing the failure path
	bool m_dpGetShouldFail = false;
	//! Whether the get port should hand out a buffer too small for a chunk
	bool m_dpGetUndersizedBuffer = false;
	//! Priority carried by the most recently sent container
	FwDpPriorityType m_dpLastPriority = 0;

[lestarch-autobot]

[C++ Design] could fix CPP-34 (bounded loops): in IMMEDIATE mode (chunkLimit == 0) the loop condition is always true, so the bound is not visible in the loop header.

Termination relies on the early returns (range exhausted, read/serialize/get failure) plus monotonic m_dpOffset progress, which holds because readSize == requestedSize > 0 is enforced. Per the JPL loop-bound rule (CPP-27/CPP-34), prefer an explicit bound in the header, e.g. compute maxChunks = (remaining range + chunkSize - 1) / chunkSize up front and loop chunk < maxChunks.

[lestarch-autobot]

[Design] must fix design-fprime-anti-pattern: IMMEDIATE mode performs unbounded ground-commanded work inside an async command handler.

For a large file, the active component's queue (including async pingIn) is blocked for the entire read + DP emission, risking health ping timeouts. Per docs/user-manual/framework/component-and-port-selection.md, event-driven work should stay bounded per dispatch; a configurable burst size (e.g. a per-request chunks-per-tick) would deliver the requested speed without starving the queue. Noting the mode was requested in PR review, so this is a deliberate trade-off needing confirmation.

cc @LeStarch @thomas-bc @timcanham — low-confidence finding, please confirm.

[lestarch-autobot]

[Design] could fix design-code-mismatch: GenerateDp never updates the component's CommandsExecuted/Errors telemetry counters.

The SDD states each command "returns success or failure and emits status information", and the other handlers call emitTelemetry(); GenerateDp (like CalculateCrc) bypasses it, so operators tracking the counters will not see these commands. Consider incrementing the counters on completion/failure for consistency.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

Automated review summary (run 1)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	1	0	0	0	1	No-Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	1	1	1	0	3	No-Go
Documentation Currency	0	3	3	0	6	Go
Design	1	0	1	0	2	No-Go
Architecture	0	1	0	1	2	Go
Test Quality	7	4	2	0	13	No-Go
Correctness	0	0	4	1	5	Go
Maintainability	0	1	5	0	6	Go
CI safety	—	—	—	—	—	Go
Totals	10	10	16	2	38	No-Go

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Outstanding must-fix items (10)

Security Vulnerabilities

• IMMEDIATE mode performs ground-controlled unbounded work in the command handler (ground-overflow-or-ddos) — #5561 (comment)

F Prime C/C++ Design

• CPP-19: new data-product bookkeeping members are uninitialized until resetDpState() runs — #5561 (comment)

Design

• design-fprime-anti-pattern: IMMEDIATE mode performs unbounded ground-commanded work inside an async command handler — #5561 (comment)

Test Quality

• GenerateDpStarted asserted by count only; payload (fileName, bytesToWrite) never checked — #5561 (comment)
• GenerateDpFailed asserted by count only; stage/status payload never checked — #5561 (comment)
• GenerateDpInvalidRange asserted by count only; offset/size payload never checked — #5561 (comment)
• GenerateDpBufferFailed asserted by count only; fileName payload never checked — #5561 (comment)
• Busy rejection asserts GenerateDpFailed by count only; stage == BUSY never checked — #5561 (comment)
• Serialization-failure test asserts GenerateDpFailed by count only; stage == SERIALIZE never checked — #5561 (comment)
• No test verifies content of emitted FileChunkHeaderRecord/FileChunkDataRecord records — #5561 (comment)

Merge readiness

Merge readiness: No-Go — 10 outstanding must-fix items across Security Vulnerabilities (1), F Prime C/C++ Design (1), Design (1), and Test Quality (7).

Agents that did not run on this PR

• None — all nine registered reviewers completed.

---

A solid data-product downlink capability taking shape — clear the flagged items and this one is go for launch.

[timcanham]

The changes I requested were made.

[durasi]

Thanks for the positive review.
Regarding the automated review from lestarch-autobot, I'm on it. I will address all the "must fix" items and push the updates as soon as possible.

[durasi]

I've addressed the eight test quality and C++ design must-fix items in e8e6b3f:

• Every GenerateDp event now asserts its full payload, not just the count: GenerateDpStarted (name + bytes), GenerateDpFailed at the OPEN, BUSY and SERIALIZE stages, GenerateDpInvalidRange offsets, and GenerateDpBufferFailed name. The open status and serialize status are recovered from the framework rather than hardcoded, so the assertions track the real values.

• The busy rejection test now drives the first generation to completion and asserts its response and GenerateDpComplete, confirming the rejection left the inprogress run intact.

• productSend_handler accumulates each container's data size, and generateDpSucceed asserts the totals, so the chunks are verified to carry real header-plus-data payloads.

• The tester's data product members have default initializers (CPP-19)

28 tests pass locally, line coverage is 88.2% (above the 88.02% baseline), and clangformat is clean.

On the two remaining must fix items (Security and Design), both concern IMMEDIATE mode doing unbounded work in the command handler. Since IMMEDIATE was added at your request for projects that want the file out quickly, I'd rather confirm the direction before changing it. Two options as I see them:

1. Keep IMMEDIATE but bound it with a per request burst cap, which keeps the speed intent while protecting the queue and health pings.

2. Drop IMMEDIATE and keep only the paced mode.

Which would you prefer, happy to implement either

[LeStarch]

@durasi this looks to be an actual failure in your new UT. Please resolve.

[durasi]

Resolved in df7186f, and the branch is rebased onto current devel.

The abort came from Fw::Buffer::setData, which now advances within an existing allocation and asserts on a null backing pointer, so it can no longer initialize a default-constructed buffer. The tester used setData plus setSize to set up the DP container buffer valid when the earlier commits were written but no longer so as devel moved. Rebasing and purging the build cache reproduced the abort locally which made the cause clear.

Both call sites now build the buffer with the Fw::Buffer constructor, keeping the undersized-buffer path the serialization-failure test relies on.

Locally: 28 FileManager tests pass, the full framework suite passes (127/127), line coverage is 88.2%, and clangformat is clean. The Dp-adjacent modules (DpManager, DpWriter, DpCatalog, Fw/Dp, Fw/Buffer) pass as well.

The force push dismissed your approval sorry about that whenever you have a moment. Could you take another look?

[timcanham]

Changes I requested were made, but overall other actions need to be finished.