Potential issue where DAG branch might be left partly processed

[pcsegal]

Hi,

There seems to be a situation where a DAG branch might be left partially unprocessed and never repaired.

• When processing a node, one of the first actions taken by processNode right after merging the delta into the set is to mark the node as processed.
• In the beginning of handleBlock, any node that is already marked as processed is skipped.

Since branches are processed starting from the head and then walking down the branch, It looks like this may lead to the following scenario: if the server crashes after a node is marked as processed, but before its children are processed, then we may end up in a situation where the branch is left unprocessed and wouldn't be repaired by the automatic repair mechanism, because it was not marked as dirty.

Is this assumption correct, or am I missing something? If it's correct, then I imagine a way to fix it could be (1) only mark the branch's head as processed at the end, or (2) save to the store the fact that processing is under way, so that it can verify that there was processing under way upon startup.

[welcome]

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

• "Priority" labels will show how urgent this is for the team.
• "Status" labels will show if this is ready to be worked on, blocked, or in progress.
• "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

[guillaumemichel]

Triage notes:

• @hsanjuan will look into it

[hsanjuan]

if the server crashes after a node is marked as processed, but before its children are processed, then we may end up in a situation where the branch is left unprocessed and wouldn't be repaired by the automatic repair mechanism, because it was not marked as dirty.

Right.

Is this assumption correct, or am I missing something? If it's correct, then I imagine a way to fix it could be (1) only mark the branch's head as processed at the end, or (2) save to the store the fact that processing is under way, so that it can verify that there was processing under way upon startup.

-> (1) opens the door to several workers processing the same branches, unfortunately, so it is no-go. Plus it is very easy to break something here.
-> (2) I think it is a good idea to keep a "bad-shutdown" mark that is set on start and removed on a clean Close(). If the mark is set during start-up, it means that the datastore "died", and the existing "dirty mark" should be set so the repair process triggers. that should cover the "server crash" scenario without being intrusive to the existing logic or causing perf impacts.