[INS-232] Fix S3 Source "panic: runtime error: index out of range" bug

[mustansir14]

Description:

Recently a bug was reported in our slack community workspace related to S3 scanning. The user faced a panic: runtime error: index out of range error, originating here.

After some investigation, and the fact that this bug surfaced after unit scans were introduced in the source in #4560, I found that this was because the source uses a single Checkpointer instance throughout it's lifetime, and since ChunkUnit can be called concurrently, this instance was being shared between concurrent runs. Since the checkpointer does not support concurrent page processing, unexpected behaviors can occur.

The solution to this is to remove the Checkpointer as a class-level attribute and use separate instances for each unit/bucket. This works for legacy scans as well because the underlying sources.Progress in the checkpointer is a pointer, which means all separate Checkpointer instances share the same sources.Progress.

Theoretically this should be enough to fix the bug, but just for safety I added a length check as well.

I didn't add a new test for this because the bug is a result of concurrency and race conditions, so is not very easy to reproduce. The existing test TestSource_ChunkUnit_Resumption_MultipleBucketsConcurrent is the test which could reproduce this, but again it's not guaranteed.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[mcastorina]

Nice find and fix! Just had one question about a timeout that was changed.

[mcastorina]

This seems like a significant difference! What's the reason it was bumped up?

[mustansir14]

Thanks for catching this, It's a mistake on my part. Some of the integration tests were timing out for me because of this so I temporarily bumped it up. I'll revert this.