Repository navigation
CfdpManager: Stage received files and commit them only after verification - #6368
Draft
nikhilred2619 wants to merge 2 commits into
Draft
nikhilred2619 wants to merge 2 commits into
nikhilred2619 wants to merge 2 commits into
Conversation
Every receive transaction started with m_keep = KEEP, and nothing on the receive side cleared it, so Engine::finishTransaction never removed a received file. A Class 1 reception whose checksum failed, whose EOF declared a different size than its Metadata, or whose File Data or EOF PDU failed to deserialize also set no error status, so it was reported with RxFileTransferCompleted. A failed Class 2 reception was reported failed and its FIN said the file was discarded, but the file was kept. Receive transactions now start with m_keep = DELETE; only a matching checksum sets KEEP. The Class 1 failure paths set FILE_CHECKSUM_FAILURE, FILE_SIZE_ERROR, PROTOCOL_ERROR or FILESTORE_REJECTION, so they are reported with RxFileTransferFailed and the file is removed. A reception ended by its inactivity timer was recycled without being finished: its file (or its temporary file in tmp_dir) was closed by recycleTransaction and left in place, and no failure event was emitted. rTick now finishes an R1 or R2 transaction with INACTIVITY_DETECTED before recycling it, so it is reported failed and its file disposed of. Add unit tests for each case and document received file disposal in the SDD. Fixes nasa#6126 Fixes nasa#6230 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tion Part of nasa#6230. Builds on nasa#6365, which reports failed receptions and removes their files but still writes received data in place, so a reception that fails has already overwritten or removed an earlier file of the same name. - Write received data to a staging file beside the destination, <dst>.cfdp-<channel>-<source EID>-<seq>.part, created exclusively. Flush it and rename it onto the destination only after the transfer is complete and its checksum passes. For Class 2 the commit precedes a FIN that reports the file as retained; a failed commit is reported as complete but discarded by the filestore. - Track the file each receive created and remove only that file when the receive ends without committing, including from Engine::handleNotKeepFile, whose receive branch would otherwise remove the destination now that dst_filename names it. Component cleanup also removes in-progress files. - Require Class 1 File Data to cover every byte of the declared size, since missing zero bytes do not change the checksum, and fail a Class 2 transfer whose File Data received before Metadata extends past the declared size. - Refuse a destination named like a receive's own staging or temp file (ASCII case-insensitive) so one transaction cannot commit onto or remove another's file. - Adapt the disposal tests to staged data, cover FileRemoveFailed through the transmit branch, document staging in the SDD and add CFDP-016.
nikhilred2619
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Description
This change builds on #6365 to prevent incomplete or failed CFDP receptions from overwriting an existing valid destination file.
The implementation introduces transaction-owned staging files so received data is verified before replacing the destination.
Changes include:
No FPP model changes are included.
Dependency
This pull request depends on #6365.
It is based on commit 2b4a0fb from #6365. Until that PR merges, the comparison against devel may include the changes introduced by #6365.
This PR is being opened as a draft while the dependency and maintainer review are pending.
Rationale
An incomplete or failed CFDP reception must not destroy an earlier valid file at the destination.
Previously, received data could be written directly to the destination before the transfer was successfully verified.
Staging the received file and committing it only after verification protects existing destination files from incomplete transfers.
Testing
The implementation and validation work reported:
Regression tests cover:
The final implementation was also reviewed, with no mandatory findings reported.
Known Limitations
Future Work
AI Usage
Generative AI tools were used during this contribution.
Claude (Anthropic) assisted with code implementation and regression testing. ChatGPT (OpenAI) helped with the description.
I actively worked on this contribution by investigating the issue, defining the expected behavior, and guiding the implementation and testing. I reviewed the technical approach, understand how the changes address the underlying problem, and take responsibility for the contribution. I personally verify the final changes and address any maintainer feedback before marking the pull request ready for review.
Contributor Checklist