fix(storage): keep graph index warm on admission rejection - #2280
fix(storage): keep graph index warm on admission rejection#2280branarakic wants to merge 1 commit into
Conversation
a99c377 to
4c93034
Compare
b22bc6c to
f6532ce
Compare
4c93034 to
bcb83a9
Compare
f6532ce to
0a6d57e
Compare
| isReplaceSubjectCapabilityRefusal, | ||
| type TripleStoreCapability, | ||
| } from './unsupported-capability-error.js'; | ||
| export { |
There was a problem hiding this comment.
🟡 Issue: Avoid exporting a second overlapping error-classification API
What's wrong
The PR adds a broader “operation never started” classifier but exports it alongside the older, narrower capability-refusal helpers. That creates two public ways to answer nearly the same question, with subtly different semantics. This is unnecessary API surface and makes the failure model harder to reason about for maintainers and consumers.
Example
A downstream caller handling replaceGraph failures now has two plausible public predicates: isReplaceGraphCapabilityRefusal(error) and isAtomicReplaceOperationNotStarted(error, 'replaceGraph'). Only one includes scheduler admission rejection, so the public API invites inconsistent cache/reconcile decisions while making both shapes harder to evolve.
Suggested direction
Do not expose the new helper unless it is intentionally the canonical contract. Prefer consolidating these predicates in the existing capability-error module, or keep the broader helper private to ChangelogStore/GraphSetIndexStore until the package API can replace the older helpers cleanly.
For Agents
Review packages/storage/src/index.ts and packages/storage/src/unsupported-capability-error.ts. Preserve the new scheduler-busy behavior, but make there be one canonical public error-classification boundary: either keep isAtomicReplaceOperationNotStarted internal to the decorators, or replace/deprecate the old per-capability helpers with a single documented classifier. Existing capability-refusal tests plus the new scheduler-busy tests should still pass.
Impact
A store scheduler queue rejection occurs before the operation closure starts. Atomic graph-replace wrappers now recognize that typed pre-execution outcome and keep their warm named-graph index instead of marking it dirty and forcing an O(store) rebuild on the next graph read.
This breaks a feedback loop observed during the Testnet 500/500 run: load caused an atomic replace to wait out its queue slot, the harmless admission rejection dirtied the index, the next read launched a full background scan, and that scan consumed capacity needed by the publication and synchronization workload.
Before
sequenceDiagram participant Workload participant Scheduler participant Index as Graph-set index participant Store Workload->>Scheduler: Atomic replace Scheduler-->>Workload: Busy before execution Workload->>Index: Next graph read Index->>Store: Full named-graph rebuild Note over Scheduler,Store: Extra scan amplifies loadAfter
sequenceDiagram participant Workload participant Scheduler participant Index as Graph-set index Workload->>Scheduler: Atomic replace Scheduler-->>Workload: Typed busy before execution Index->>Index: Preserve warm index Workload->>Index: Next graph read Index-->>Workload: Serve cached graph setSafety
StoreSchedulerBusyErroris documented and enforced as retry-safe: it is raised while work remains queued, before the operation closure starts. Indeterminate execution failures still dirty the index and rebuild fail-closed.Validation