Retire oxigraph-worker backend#1539
Conversation
| ...resolveNetworkDefaultContextGraphs(network), | ||
| ]), | ||
| ]; | ||
| const effectiveStore = config.store ?? { backend: 'oxigraph-server', options: {} }; |
There was a problem hiding this comment.
🟡 Issue: Move store default/migration policy out of the daemon lifecycle body
What's wrong
This PR adds more storage migration/defaulting policy directly into an already very large orchestration function. The implementation preserves behavior, but it makes the function more stateful and harder to reason about because the store invariant is represented by several similarly shaped config aliases rather than one explicit boundary object.
Example
A future boot-time step added after this block has to choose between config, effectiveConfig, runtimeStoreConfig, and runtimeStore. That choice encodes the subtle invariant: omitted persisted store means managed oxigraph-server, but the persisted object should not necessarily be mutated to that runtime view.
Suggested direction
Introduce a single resolveDaemonStoreRuntime/boot-plan abstraction that returns named views such as operatorConfig, effectiveConfig, runtimeConfig, and runtimeStore, and move the reset/legacy checks there. That would delete several ad-hoc branches from runDaemonInner and make later call sites consume one explicit contract.
For Agents
Look in packages/cli/src/daemon/lifecycle.ts around daemon boot store setup. Extract a focused store boot-plan helper/module that resolves the omitted-store default, legacy store.nq acknowledgement, backend-switch input, managed-server runtime config, and validation config. Preserve current behavior for blockless configs, explicit oxigraph-worker, legacy store.nq, and managed oxigraph-server; prove it with the existing daemon/store tests plus focused unit tests for the resolver.
There was a problem hiding this comment.
🟡 Issue: Separate persisted config from the materialized store default
What's wrong
This change creates two mutable DkgConfig views in the daemon: the operator config and a default-materialized effective config. Routing code treats its config as the object to persist, so store-default materialization now leaks across an unrelated persistence boundary. That makes future route work harder to reason about because authors must know which config view they received before saving it.
Example
A node with no persisted store block boots with the effective default, then an unrelated route such as /api/local-agent-integrations/connect saves ctx.config. Because that context now receives storeBoot.effectiveConfig, the unrelated save path serializes the materialized default store block too. Even if that write is acceptable, the route layer cannot tell whether it is mutating the operator config or a runtime/defaulted view.
Suggested direction
Do not pass a default-materialized DkgConfig as the general mutable config object. Keep the original config for route handlers that may call saveConfig, and pass a small explicit store/runtime view for status, health, or agent-only consumers.
For Agents
Inspect packages/cli/src/daemon/lifecycle.ts around the handleRequest call and the RequestContext.config save paths. Preserve blockless daemon boot/status behavior, but keep persistence routes on the original operator config and pass store runtime/status data through an explicit store context. Add a focused test where a blockless config is saved by an unrelated route without relying on the materialized store default.
| const DEFAULT_STORE_BACKEND = 'oxigraph-server'; | ||
| const UNSUPPORTED_WORKER_BACKEND = 'oxigraph-worker'; | ||
|
|
||
| function isSupportedExistingBackend(backend: string | undefined): backend is string { |
There was a problem hiding this comment.
🟡 Issue: Replace parallel backend conditionals with one typed backend policy
What's wrong
The PR adds a new backend policy surface, but represents it as several separate string lists and special-case branches. That makes the code harder to maintain because the accepted backends, displayed choices, preservation rules, and error messages can drift independently.
Example
oxigraph-persistent is treated as a supported existing backend and has custom handling in both prompt and flag flows, but the generic error messages omit it. That mismatch is a symptom of the backend policy being spread across parallel conditionals instead of modeled once.
Suggested direction
Use a small declarative model, for example STORE_BACKENDS[backend] = { retired, menu, local, requiresUrl, requiresExistingPath, preserveOptions }, and have both promptStoreBackend and applyStoreFlagsToConfig call shared parse/resolve helpers. This would make the retired-worker case and future backend changes one-table edits instead of scattered branches.
For Agents
Refactor packages/cli/src/store-wizard.ts around backend parsing and flag application. Create a typed backend policy table for oxigraph-server, oxigraph, oxigraph-persistent, blazegraph, sparql-http, and retired oxigraph-worker; derive menu choices, accepted names, preservation behavior, URL requirements, path requirements, and error text from that table. Preserve current prompt/flag behavior and keep the existing store-wizard tests as the behavioral guard.
There was a problem hiding this comment.
🟡 Issue: Make backend policy canonical instead of wizard-local
What's wrong
The new STORE_BACKENDS table is useful, but placing it only in the wizard leaves backend lifecycle policy scattered across the codebase. The table, storage helpers, config validation, and command help all encode overlapping facts about supported, retired, local, external, and menu-visible backends. This increases maintenance cost for exactly the kind of migration this PR is doing and keeps casts/stringly checks in the flow instead of giving the rest of the CLI a clear typed boundary.
Example
applyStoreFlagsToConfig can accept --store oxigraph-persistent when an existing path is present, and unknown-backend errors advertise it via supportedBackendList(). The user-facing option help added in the command files omits it. That is a small symptom of the larger issue: backend support policy now has to be updated in multiple places.
Suggested direction
Extract a single typed backend policy boundary, with storage-owned backend families where possible and CLI-only menu labels as an overlay. Then consume it from the wizard, flag validation, config validation, and command help text instead of keeping parallel string lists.
For Agents
Look at packages/cli/src/store-wizard.ts, packages/cli/src/config.ts, the command option descriptions, and storage backend helpers. Preserve the same accepted/rejected backends and retired worker messaging, but move backend support/menu/description data into one typed policy module or derive CLI text from the parser policy. Add a test that the advertised backend list and parser-supported backend list cannot drift.
| exitOnStoreConfigErrors(config, log); | ||
| } | ||
|
|
||
| if (!config.store && existsSync(join(dkgDir(), 'store.nq')) && !acceptStoreReset) { |
There was a problem hiding this comment.
🟡 Issue: The legacy store.nq boot gate is not covered by daemon startup tests
What's wrong
This is the PR’s main data-safety migration behavior: a block-less legacy node with an old worker store.nq should stop unless the operator explicitly acknowledges the fresh-store cutover. The new unit tests cover config validation, wizard handling, and storage factory rejection, but not the actual daemon boot branch that enforces the safeguard. A regression here could either silently start a fresh managed store over legacy data or block an operator even after setting the opt-in flag.
Example
A focused regression test could set DKG_HOME to a temp dir, write store.nq, call runDaemonInner() with no config.store, mock process.exit, and assert it exits with the legacy-store message before agent creation. A second case should set DKG_ACCEPT_STORE_RESET=1 and assert boot proceeds using the effective oxigraph-server config.
Suggested direction
Add integration-style daemon startup tests for the new migration gate and its opt-in bypass, with the managed Oxigraph startup mocked so the assertions stay deterministic.
Confidence note
I could not run the targeted test because pnpm is not installed in this environment; the finding is based on the diff and repository test search.
For Agents
Add daemon startup tests around runDaemonInner in packages/cli/test/daemon-startup-validation.test.ts or a sibling test. Cover both store.nq present with no DKG_ACCEPT_STORE_RESET (must exit before managed Oxigraph/agent startup) and DKG_ACCEPT_STORE_RESET=1 (must continue and pass an effective store.backend: "oxigraph-server" into the managed-store path).
| @@ -162,10 +110,8 @@ function resolveStoreSizeLabels(): string[] { | |||
| return labels; | |||
There was a problem hiding this comment.
💡 Suggestion: Collapse the single-backend benchmark scaffolding
Why it matters
The current version keeps the shape of the removed worker comparison, including env parsing and an ignored parameter. That is small but unnecessary indirection, and it makes the benchmark look more configurable than it is.
Suggestion
Now that the worker backend is retired, delete the backend abstraction rather than leaving a one-value mode system. The benchmark can be a direct in-process Oxigraph read-latency suite until another real backend comparison exists.
branarakic
force-pushed
the
codex/remove-oxigraph-worker
branch
from
4894803 to
b842ba8
Compare
|
Addressed the review feedback in b842ba8:
Verification run locally:
|
| validateOperatorConfigFirst: config.store?.backend === 'oxigraph-worker', | ||
| }; | ||
|
|
||
| if (!config.store && existsSync(legacyStorePath) && !acceptStoreReset) { |
There was a problem hiding this comment.
🔴 Bug: Explicit legacy worker configs can bypass the store reset acknowledgement
What's wrong
The new safety gate only protects blockless configs. Once a legacy explicit oxigraph-worker config is rewritten to an explicit oxigraph-server block, the daemon no longer checks for the old store.nq before starting the fresh managed store. If the backend-switch state does not already contain lastBackend, this can make existing data disappear from the running node without the reset acknowledgement this PR is trying to enforce.
Example
A legacy node has config.store.backend = "oxigraph-worker" with ~/.dkg/store.nq containing data, and its chain-reset state file has no lastBackend because it predates that marker. The operator runs dkg init and presses Enter at the store prompt; the wizard writes { backend: "oxigraph-server" }. On boot, config.store is now present, so the store.nq gate is skipped; detectBackendSwitch sees previous === null and silently records oxigraph-server. The daemon starts on an empty managed RocksDB store without requiring DKG_ACCEPT_STORE_RESET=1.
Suggested direction
Do not limit the legacy store.nq gate to blockless configs. Either gate any oxigraph-server cutover when a legacy worker store file is present and no prior backend marker proves acknowledgement, or make the wizard refuse/require explicit confirmation for existing oxigraph-worker configs instead of writing oxigraph-server automatically.
For Agents
Look at resolveDaemonStoreBootPlan and promptStoreBackend's existingBackend === 'oxigraph-worker' path. Preserve/detect the legacy worker persistence path, including default dataDir/store.nq, and require reset acknowledgement or refuse the auto-rewrite before an explicit oxigraph-server block can bypass the boot gate. Add a test for explicit legacy oxigraph-worker config plus store.nq plus missing lastBackend proving init/boot cannot silently cut over.
| }); | ||
| expect(mocks.agentCreate).toHaveBeenCalledTimes(1); | ||
| expect(mocks.agentCreate.mock.calls[0]?.[0]).toMatchObject({ | ||
| storeConfig: { backend: 'oxigraph-server', options: {} }, |
There was a problem hiding this comment.
🔴 Bug: The acknowledged legacy-store test asserts a stubbed backend instead of the runtime backend
What's wrong
The new boot test is meant to verify that a blockless config with legacy store.nq can continue after DKG_ACCEPT_STORE_RESET=1, but it stubs away the managed Oxigraph result. That means it never verifies the critical behavior that oxigraph-server is converted into the runtime sparql-http store before agent creation, and it gives false confidence for a user-facing startup path.
Example
If lifecycle stopped using managed.storeConfig and passed { backend: 'oxigraph-server' } to DKGAgent.create, this test would still pass because DKGAgent.create is mocked. In production, storage does not register oxigraph-server as a TripleStore backend, so the agent path should receive the managed runtime sparql-http config instead.
Suggested direction
Have the mock model a successful managed Oxigraph startup and assert the runtime handoff to the agent, not the pre-managed effective config.
For Agents
Update the acknowledged legacy-store daemon startup test in packages/cli/test/daemon-startup-validation.test.ts so startManagedOxigraph returns a ManagedOxigraphResult with storeConfig { backend: 'sparql-http', options: { managedByDkg: true, queryEndpoint, updateEndpoint } }. Keep the assertion that startManagedOxigraph receives the effective oxigraph-server config, but assert DKGAgent.create receives the rewritten runtime store and managed blob/snapshot values.
|
|
||
| return await createTripleStore({ | ||
| backend: 'oxigraph-worker', | ||
| backend: 'oxigraph-persistent', |
There was a problem hiding this comment.
🟡 Issue: The standalone publisher default store fallback is not exercised
What's wrong
The PR changes the publisher runner's implicit store from the removed worker backend to oxigraph-persistent, but the tests only cover publisher runtime creation when config.store is set explicitly. This leaves the standalone publisher CLI's blockless/default path unverified even though the removed backend would make that path fail at bootstrap.
Example
A regression that left the fallback as createTripleStore({ backend: 'oxigraph-worker', ... }) would now fail with the storage-layer retired-backend error, but the current createPublisherRuntime tests would not catch it because they pass store: { backend: 'oxigraph' }.
Suggested direction
Cover the no store block publisher runtime path with a regression test that would fail if it used the retired oxigraph-worker backend or omitted the persistent path.
For Agents
Add a publisher-runner test, likely in packages/cli/test/publisher-wallets.test.ts, that creates a publisher wallet and calls createPublisherRuntime with a config that omits store. Assert it boots and closes cleanly, or assert the default persistent store path is usable, so the retired-worker fallback would fail the test.
2 participants
Summary
Verification