fix(cli): mount the Blazegraph journal on a named volume, and never guess its name - #2093
fix(cli): mount the Blazegraph journal on a named volume, and never guess its name#2093branarakic wants to merge 1 commit into
Conversation
…uess its name
The daemon's container-recovery path can destroy a node's entire graph.
`provisionBlazegraphDocker` creates the Blazegraph container with
`-d --restart --name -p <image>` and no `-v`, so the journal
(`/data/bigdata.jnl`, pinned by the image in
`WEB-INF/classes/RWStore.properties`) lives in the container's writable
layer. Step 3 of the same function does `docker rm -f` whenever a stopped
container fails to `docker start`, then falls through to that create path.
The result is silent, total data unavailability for the node: it comes back
serving zero triples.
This is reachable on mainnet today. The V10 fleet runs with
`store.options.managedByDkg = true`, and its containers were migrated to a
named volume out-of-band, so the journal survives — but the daemon does not
know about it and would recreate without mounting it, orphaning 11-18 GB of
graph while starting an empty store.
Two changes:
1. Always mount a named volume at `/data` on create. `docker run -v` creates
the volume on demand and reuses it (with its contents) when it exists, so
this is safe for both fresh and previously-provisioned hosts.
2. Resolve the volume from the container's own `.Mounts`, never from the
container name. The fleet is the reason: containers are named
`dkg-blazegraph-dkg` while the journal volume is `dkg-blazegraph-data`, so
a `${containerName}-data` derivation yields `dkg-blazegraph-dkg-data` —
a volume that does not exist, which docker would helpfully create empty.
The container is the only authoritative source for where its journal is.
Also fail closed rather than recreating over a mount we cannot reproduce: if
something non-volume (e.g. an operator bind mount) is mounted at `/data` and
the container will not start, throw instead of `rm -f`. Recovering one
stopped container by hand is strictly cheaper than silently emptying a store.
Tests cover the fresh-create mount, the fleet's container/volume name
mismatch, and the bind-mount refusal (asserting no `rm` and no `run` are
issued). Verified by mutation: reintroducing the `${containerName}-data`
derivation fails two of them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // Journal volume used by any container created below. An existing container | ||
| // is authoritative about where its own journal lives; the default name is | ||
| // only for a store that does not have one yet. | ||
| const journalVolume = inspectInfo.dataVolume ?? DEFAULT_BLAZEGRAPH_VOLUME; |
There was a problem hiding this comment.
🔴 Bug: Fresh containers all share the same Blazegraph journal volume
What's wrong
DEFAULT_BLAZEGRAPH_VOLUME is used whenever the target container does not already exist or lacks an inspectable named volume. Because the provisioner supports namespace-specific container names and port auto-bumping, two independently provisioned nodes on the same Docker host will mount the same named volume at /data. Blazegraph's journal is a single durable store file, so sharing it between independent containers can expose the wrong namespaces and risks concurrent journal corruption.
Example
Call provisionBlazegraphDocker({ namespace: 'alice' }), then call provisionBlazegraphDocker({ namespace: 'bob', port: 10000 }) while the first container still exists. The second inspect checks dkg-blazegraph-bob, sees no container, and runs a second Blazegraph JVM with -v dkg-blazegraph-data:/data. Both containers now share and can write the same /data/bigdata.jnl; expected behavior is isolated journals per independent container, or refusing to attach an in-use/shared journal.
Suggested direction
Separate the volume-selection path for existing-container recovery from fresh provisioning. Preserve mounted volumes discovered from .Mounts, but do not use one global writable volume for every new namespace/container; either scope the default volume to the namespace/container or make sharing an explicit, validated option.
For Agents
In packages/cli/src/daemon/blazegraph-docker.ts, keep inspectInfo.dataVolume authoritative when recreating an existing container, but make the default for a truly fresh container unique to that logical store, or explicitly detect/refuse when the default volume is already attached to another container. Add a regression test that provisions two different namespaces/container names and proves they do not receive the same writable /data volume unless the caller explicitly requests sharing.
| // `dkg-blazegraph-dkg-data`, which does not exist — docker would create it | ||
| // empty and the node would come up serving zero triples while the real | ||
| // 11-18 GB journal sat orphaned. | ||
| const FLEET_VOLUME = 'dkg-blazegraph-data'; |
There was a problem hiding this comment.
🔴 Bug: The existing-volume regression test does not prove the inspected volume is used
What's wrong
This test is intended to verify that recreate uses the volume discovered from docker inspect, but the mocked volume name equals the default volume. That gives false confidence for the main changed behavior: a regression that stops reading the container's .Mounts would still be green.
Example
Change the production code to const journalVolume = DEFAULT_BLAZEGRAPH_VOLUME; and this test still passes, so it does not prove that an existing container's inspected Mounts[].Name is reused.
Suggested direction
Use a non-default inspected volume name, for example operator-blazegraph-journal, so the test fails if the implementation falls back to the default instead of reading .Mounts.
For Agents
In packages/cli/test/blazegraph-docker.test.ts, make the inspected volume name distinct from DEFAULT_BLAZEGRAPH_VOLUME and assert docker run -v uses that exact custom name. Preserve the separate assertion that the container-name-derived dkg-blazegraph-dkg-data is not used.
| * container has one. This is the ONLY trustworthy way to learn where an | ||
| * existing container's journal lives. | ||
| */ | ||
| dataVolume?: string; |
There was a problem hiding this comment.
🟡 Issue: Model the journal mount as one explicit state instead of loose optional flags
What's wrong
The PR introduces a real domain concept, "what journal store should a replacement container use?", but represents it as two independent optional fields plus a later defaulting expression and a separate guard. That spreads one invariant across comments and control flow, making future edits easier to get subtly wrong and forcing readers to reconstruct which combinations are valid.
Example
The current shape allows impossible or ambiguous states such as { dataVolume: 'custom', foreignDataMount: true } or { dataVolume: undefined, foreignDataMount: undefined }, and the recreate path has to remember that one means reuse/refuse/default depending on context. A discriminated model would make those states unrepresentable.
Suggested direction
Move the mount interpretation into a small helper or discriminated union such as { kind: 'named-volume'; name } | { kind: 'foreign' } | { kind: 'default' }. Then make the recreate path switch on that value. This deletes the parallel optional fields and keeps the volume-selection policy in one place.
For Agents
In packages/cli/src/daemon/blazegraph-docker.ts, replace dataVolume?: string and foreignDataMount?: boolean with a single journal mount model returned by inspect parsing, e.g. named-volume, foreign, or default/missing. Preserve behavior: reuse the inspected named volume on recreate, use DEFAULT_BLAZEGRAPH_VOLUME for fresh creates, and refuse to recreate non-volume mounts after docker start fails. Existing volume tests should still pass, with one focused test proving the foreign state cannot fall through to docker run.
|
Closing as superseded by #2086 / #2137, which are now merged to main ( The data-loss path this PR targets no longer exists on main:
One idea from this PR is deliberately not on main: resolving the volume from the container's own |
Summary
The daemon's Blazegraph container-recovery path can silently destroy a node's entire graph. This fixes it.
provisionBlazegraphDockercreates the container with-d --restart --name -p <image>and no-v(
blazegraph-docker.tsstep 4),so the journal lives in the container's writable layer. The image pins
com.bigdata.journal.AbstractJournal.file=/data/bigdata.jnlinWEB-INF/classes/RWStore.properties, so everythingdurable is under
/data.Step 3 of the same function issues
docker rm -fwhenever a stopped container fails todocker start, and thenfalls through to that create path:
The node comes back serving zero triples, with no error surfaced.
Why this is not hypothetical
The V10 mainnet fleet runs with
store.options.managedByDkg = true(verified on sbb and dmaast), so this code ownsthose containers. The fleet's journals were migrated to a named volume out-of-band:
So the data currently survives — but only by luck. The daemon doesn't know the volume exists and would recreate
without mounting it, orphaning 11–18 GB of graph per node behind an empty store.
Changes
1. Always mount a named volume at
/dataon create.docker run -v <name>:<path>creates the volume on demandand reuses it with its contents when it already exists, so this is correct for both fresh and already-provisioned
hosts.
2. Resolve the volume from the container's own
.Mounts, never from the container name. This is the importantone. The fleet's containers are named
dkg-blazegraph-dkgwhile the volume isdkg-blazegraph-data— so a`${containerName}-data`derivation producesdkg-blazegraph-dkg-data, a volume that does not exist and whichdocker would helpfully create empty. The container is the only authoritative source for where its own journal
lives.
3. Fail closed instead of recreating over a mount we cannot reproduce. If something non-volume (e.g. an operator
bind mount) is at
/dataand the container won't start, throw rather thanrm -f. Recovering one stopped containerby hand is strictly cheaper than silently emptying a store.
Testing
Three new tests in
packages/cli/test/blazegraph-docker.test.ts:dkg-blazegraph-data:/dataand keeps the image as the final argument/data→ throws, and asserts normand norunwere issued22 passedin the file;tsc --noEmitclean.Mutation-verified: reintroducing
const journalVolume = `${containerName}-data`fails two of the three newtests. The guards have teeth rather than merely passing.
Risk
Low. Behaviour is unchanged for a running container (the reuse path is untouched). The only new failure mode is the
deliberate fail-closed throw, which replaces a silent data-availability loss with an actionable error.
Existing fleet containers already mount
dkg-blazegraph-data:/data, so this change is a no-op for them untilsomething triggers a recreate — which is precisely the case it makes safe.