feat: make indexing service integration optional - #28
Merged
Conversation
Deployments without an indexing service (e.g. Forge staging) had no way to turn the integration off: config validation required the indexer DID/URL and at least one IPNI announce URL, and blob/accept fails hard when the configured indexer is unreachable (publishing the location commitment POSTs claim/cache synchronously). Mirror the egress-tracker disable pattern: leaving the indexer DID or URL empty now disables the integration with a warning, and ipni_announce_urls may be empty (adverts are still built and served locally, just not announced). The no-op seam already existed — publisher.CacheClaim returns early when the indexer DID is undefined — it was just unreachable through config validation. Also allow a zero max_batch_size_bytes on the egress tracker config: zero already means the etracker is disabled, piri init always writes the default, and Normalize() bumps sub-minimum positive values, so nothing relied on rejecting 0. Assisted-by: Claude:claude-fable-5 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Makes external indexing-service integration configurable/optional so deployments can run without an indexer (and without IPNI announce URLs), while preserving existing no-op behavior in the publisher when the indexer is unset.
Changes:
- Relax config validation so the indexing service is disabled when DID/URL are empty, and
ipni_announce_urlscan be empty. - Add warnings + early-return behavior in
IndexingServiceConfig.ToAppConfig()when indexer config is incomplete/omitted. - Allow
etracker.max_batch_size_bytes = 0by updating validation, and add unit tests for the new optional-indexer semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/config/services.go | Makes indexer/publisher config optional via validation changes and disables indexer integration when DID/URL are empty. |
| pkg/config/services_test.go | Adds coverage for “indexer disabled” validation and empty IPNI announce URLs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alanshaw
requested changes
Jul 20, 2026
| // According to the spec, batch size should be between 10MiB and 1GiB | ||
| // (see https://github.com/storacha/specs/blob/main/w3-egress-tracking.md) | ||
| MaxBatchSizeBytes int64 `mapstructure:"max_batch_size_bytes" validate:"min=10485760,max=1073741824" flag:"egress-tracker-service-max-batch-size-bytes" toml:"max_batch_size_bytes,omitempty"` | ||
| MaxBatchSizeBytes int64 `mapstructure:"max_batch_size_bytes" validate:"omitempty,min=10485760,max=1073741824" flag:"egress-tracker-service-max-batch-size-bytes" toml:"max_batch_size_bytes,omitempty"` |
Member
There was a problem hiding this comment.
What is the semantic change here? Does an explicit 0 mean "use the default" or is it "disable egress tracking"? Can we make clear in comments/help text?
Member
Author
There was a problem hiding this comment.
Requested in PR #28 review: pin down that a zero max_batch_size_bytes passes validation (both on the struct and via ServicesConfig), and that the 10MiB-1GiB spec range is still enforced for non-zero values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDZNkKCMmnXxwwKWYkQ6BQ
Answer the PR #28 review question about what an explicit 0 means: it selects the default batch size, it does not disable egress tracking — the integration is disabled by leaving the etracker DID/URL empty. State this in the config field comment, the hidden serve flag help text, and the configuration docs. Also correct the documented default: piri serve injects DefaultMinimumEgressBatchSize (10485760, 10 MiB) via the flag default, not 104857600 (100 MiB). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDZNkKCMmnXxwwKWYkQ6BQ
Follow-up to the Copilot review on 61e1223: an unset max_batch_size_bytes and an explicit 0 are not equivalent under `piri serve` — an unset key picks up the 10MiB flag default, while an explicit 0 flows through to the retrieval journal, which substitutes its built-in 100MiB batch size. Describe that in the field comment, flag help text, and docs instead of calling both "the default". Also fix the IndexingServiceConfig comment: the integration is disabled when either the DID or the URL is empty, not only when both are. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDZNkKCMmnXxwwKWYkQ6BQ
alanshaw
approved these changes
Jul 21, 2026
| "egress-tracker-service-max-batch-size-bytes", | ||
| config.DefaultMinimumEgressBatchSize, | ||
| "Maximum batch size in bytes for egress tracker service. It should be between 10MiB and 1GiB", | ||
| "Maximum batch size in bytes for egress tracker service, between 10MiB and 1GiB. An explicit 0 selects the retrieval journal's built-in default (100MiB) rather than this flag's default; it does not disable egress tracking (leave the egress tracker DID/URL empty to disable).", |
Member
There was a problem hiding this comment.
Perhaps the default for this flag should also be 100MiB so this is less confusing?
Member
Author
There was a problem hiding this comment.
I agree the current defaults are confusing. Let's defer further improvements to follow-up pull requests, please; I'd like to get this one landed ASAP.
| applies the 10MiB default shown above, while an explicit `0` makes the | ||
| retrieval journal fall back to its built-in batch size of 100MiB. Neither | ||
| disables egress tracking — the integration is disabled by leaving `did` and | ||
| `url` unset. |
Member
There was a problem hiding this comment.
I think we should consider having an explicit enabled config option when a feature is not mandatory.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deployments without an indexing service (e.g. Forge staging) had no way to turn the integration off: config validation required the indexer DID/URL and at least one IPNI announce URL, and blob/accept fails hard when the configured indexer is unreachable (publishing the location commitment POSTs claim/cache synchronously).
Mirror the egress-tracker disable pattern: leaving the indexer DID or URL empty now disables the integration with a warning, and
ipni_announce_urlsmay be empty (adverts are still built and served locally, just not announced). The no-op seam already existed —publisher.CacheClaimreturns early when the indexer DID is undefined — it was just unreachable through config validation.Also allow a zero
max_batch_size_byteson the egress tracker config: zero already means the etracker is disabled, piri init always writes the default, andNormalize()bumps sub-minimum positive values, so nothing relied on rejecting 0.