Skip to content

fix: find tasks shouldn't poll providers#14

Open
volmedo wants to merge 1 commit intomainfrom
vic/fix/find-should-not-poll
Open

fix: find tasks shouldn't poll providers#14
volmedo wants to merge 1 commit intomainfrom
vic/fix/find-should-not-poll

Conversation

@volmedo
Copy link
Member

@volmedo volmedo commented Jun 8, 2025

Context

The registry component, used by both find and ingest tasks to store provider information, has the ability to poll providers periodically to confirm they are still alive, removing them if they cannot be contacted in a given amount of time.

In Storacha's deployment, find tasks are meant to read from the registry, while the ingest task will update it as it receives announcements and ads. There is no point in find tasks polling providers, as they are not in charge of keeping the registry up to date.

Proposed Changes

Disable provider polling in the registry configuration for find tasks.

Tests

Unit tests.

@volmedo volmedo self-assigned this Jun 8, 2025
@volmedo volmedo changed the base branch from main to vic/feat/reload-registry June 9, 2025 09:41
@volmedo volmedo changed the base branch from vic/feat/reload-registry to main June 9, 2025 09:49
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

We probably do want to clean up dead providers - perhaps create an issue to move this process from the finder to ingestion?

"PollRetryAfter": "5h0m0s",
"PollStopAfter": "168h0m0s",
"PollOverrides": null,
"PollInterval": "0",
Copy link
Member

Choose a reason for hiding this comment

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

What is the consequence if they do? Perhaps a comment here to explain why this is set to zero will help our future selves.

Copy link
Member Author

Choose a reason for hiding this comment

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

the only consequence is that providers would receive pings from all the find tasks we deploy. Not that much of an issue, but unnecessary.

That code needs to be valid JSON so I can't just add a comment. Do you think adding a new _comment field would be worth it?

@volmedo
Copy link
Member Author

volmedo commented Jun 9, 2025

We probably do want to clean up dead providers - perhaps create an issue to move this process from the finder to ingestion?

that is what we do. The ingest task will still poll providers and remove them when they don't reply to the ping. This is where the ingester removes entries for deleted providers, after receiving the hint from the registry through the registry.SyncChan().

@volmedo
Copy link
Member Author

volmedo commented Jun 9, 2025

on the second read I did I saw that the registry not only deletes providers that are unresponsive for a while, but it also can mark them as inactive, which seems like a previous step to deleting them from which providers can recover. However, it only marks them as inactive in memory, and I'm failing to see where that information is persisted to the datastore.

The idea is that inactive providers would also not appear in find queries, but if that info is not persisted our find tasks will still return the corresponding results.

Not sure if that would be a big issue, but maybe it's better to keep the find tasks polling providers on their own. What do you think?

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.

2 participants