objstore.DownloadDir() doesn't correctly cleanup on failure

[pracucci]

What happened:
We've seen an error like this:

msg="failed to remove file on partial dir download error" file=/data/compact/0@5679675083797525161-1614499200000-1614506400000/01EZM4R375DDZC1F52B9HV7S8G err="remove /data/compact/0@5679675083797525161-1614499200000-1614506400000/01EZM4R375DDZC1F52B9HV7S8G: directory not empty"

From a quick look at the DownloadDir() implementation (code) I think it doesn't cleanup cleanup on failure when the downloaded directory contains subdirectories too (eg. block chunks/).

I see a couple of issues:

1. downloadedFiles is not populated when DownloadDir() is called recursively
2. downloadedFiles is populated with the dst and not actually the downloaded file, but then we delete it we call os.Remove() (non recursive)

What you expected to happen:

Fixing the logic should be easy, but I'm not sure what's the desired behaviour. Do we expect DownloadDir() to delete everything including subdirectories?

[bwplotka]

Ideally it deletes all it downloaded if requested. WDYT?

It's sounds like a bug.

[GiedriusS]

Could you please print the whole directory tree of when this happens if possible?

[pracucci]

Ideally it deletes all it downloaded if requested. WDYT?

I do agree. I'm actually thinking whether it's easier to change DownloadDir() logic and not make the use of any recursion. Now we have the ability to Iter() objects recursively, so we could just leverage on it. This would simplify the fix.

[pracucci]

Could you please print the whole directory tree of when this happens if possible?

Yeah. Unfortunately happened on a customer cluster and I just got this report (I wasn't actively debugging it).

[GiedriusS]

Could you please print the whole directory tree of when this happens if possible?

Yeah. Unfortunately happened on a customer cluster and I just got this report (I wasn't actively debugging it).

I presume your customer doesn't run code from v0.19.0-rc.1? So, this issue is probably from v0.18.0 i.e. before #3031 ?

[saswatamcode]

I think this bug would arise when the downloadedFile array is populated with a dst that hasn't been downloaded. This possibly should cause DownloadFile to return an error though and the array wouldn't be appended. Was there any other error just before this? It would be easier to figure out the flow then.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗