Skip to content

fix: increase default dataset sync timeout - #109

Merged
Patai5 merged 2 commits into
masterfrom
fix/increase-default-dataset-sync-timeout
Aug 6, 2026
Merged

fix: increase default dataset sync timeout#109
Patai5 merged 2 commits into
masterfrom
fix/increase-default-dataset-sync-timeout

Conversation

@Patai5

@Patai5 Patai5 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Turns out that the previous ten seconds are also not enough in some rare cases. Increasing to 20s should fix this.

I've decided to also make this option modifiable by the user, but I'm not quite sure about passing it down the arguments of the functions like this. Let me know your thoughts.


Fixes https://github.com/apify-store/amazon/issues/687

@ruocco-l

ruocco-l commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

20 seconds as a default wait I think it's a crazy long time. I think that we should escalate this to platform, instead of punishing us by waiting (some test run will be shorter than this waiting time!).

Also, I don't particularly like the custom option, it's not clear and again, it's not something that we should care about: 10s is a soft "just in case" buffer, it must not be "we have to wait otherwise it does not work". If that's the case, there is something wrong and we need to report it, not accept it and work around it.

@ruocco-l

ruocco-l commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Also, if the problem is a crazy fast run, you can retry the test multiple time. It's not a guarantee of success, but the same is for the delay.

@Patai5

Patai5 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

The thing is, the platform is just eventually consistent. In the docs it's recommended to wait around 10s minimum. So waiting 20s to be 99.99% sure it's synced doesn't seem like a big deal to me.

But we can also leave it at the previous 10s, but expose the option so we can adjust it to our liking in video team 😄

For completed runs, aggregated fields such as stats or dollar usage totals are eventually consistent and update within a few seconds. For values that must match finalized totals, wait about 10 seconds after the run completed, then fetch the run again.
https://docs.apify.com/api/v2/actors-actor-runs

I would of course rather have the platform fix this and be strongly consistent but I don't think that's anything easy on their end to do so. It would take months minimum.


Also, I don't particularly like the custom option, it's not clear

Wdym? The naming or the description isn't?


10s is a soft "just in case" buffer, it must not be "we have to wait otherwise it does not work".

That's exactly what it is though. We do have to wait because of the platform.

@Patai5

Patai5 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

I will open up a thread in product insights though 🤔

@Patai5

Patai5 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@metalwarrior665 metalwarrior665 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

20 seconds as a default wait I think it's a crazy long time. I think that we should escalate this to platform, instead of punishing us by waiting (some test run will be shorter than this waiting time!).

Platform won't solve this since this would require complete revamp of the whole backend. They might do it in some future but it is a huge project.

Also, I don't particularly like the custom option

Agree with Luigi, this should not be an option because it is not really a configuration thing, it is required to function.

Also, if the problem is a crazy fast run, you can retry the test multiple time. It's not a guarantee of success, but the same is for the delay.

There is already default 1 retry for tests. That will mostly work but doesn't solve the problem since it can happen for long running scrapes too.

My opinions:

  1. Default 20 sec delay is ok-ish. Tests run in parallel so in the end this should be only + 10 sec wait. Not great but not terrible.
  2. I did some clauding and it offered few workaround hacks that would probably be better solution:
    a. Load with clean: false and `fields: ['nonexistent'] which gives you array of empty objects. This still requires the platform to load everything so adds some delay and overhead for very large datasets (rare in tests)
    b.Do binary search using limit=1 and offset=N where we set N to known count + some pivot and then either binary up or binary down. This is probably worse delay than just reading all items empty but we could test it.
    c. Platform fix I shared to Slack: we should fix X-Apify-Pagination-Total header which can then be used to get real time dataset count. Currently, it is taken from Mongo itemCount but we already have access to DatasetItemsStream that computes the real live count from Redis/S3. If this would be possible fix, we could do limit=1 and then read it from the header.

@Patai5

Patai5 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Default 20 sec delay is ok-ish. Tests run in parallel so in the end this should be only + 10 sec wait. Not great but not terrible.

So 20 seconds default it is then 👍


2.a. and 2.b. are both just complicated workarounds that both just try to accomplish the same thing as increasing the timeout already does.
2.c. fixes the problem only for the dataset item count, but not for the items directly. You could get into another problem where you would read the items count to be e.g. 1, but then read the items and get an empty array. A longer timeout (20s) should fix this again for 99.99% percentile.

@metalwarrior665

Copy link
Copy Markdown
Member

One thing I forgot in my analysis is that these hacks would only help with dataset items but not with events. So for that we need the delay anyway.

@Patai5
Patai5 requested a review from metalwarrior665 August 6, 2026 12:48
@Patai5
Patai5 merged commit 9e61cce into master Aug 6, 2026
10 checks passed
@Patai5
Patai5 deleted the fix/increase-default-dataset-sync-timeout branch August 6, 2026 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants