Repository navigation
feat(storage): give every campaign its own database - #83
Conversation
Phase 2 of the multi-user plan. Phase 1 established a verified tenant; this consumes it. getDb() now resolves a SQLite file per campaign under RPG_DATA_DIR/campaigns/<shard>/<campaignId>.db, keyed on the campaign id from the signed tenant context. Isolation becomes physical rather than a predicate every query has to remember: a campaign own rows are the only rows in the file, so a query that forgets to scope cannot reach another tenant data. The 30 repositories and 217 call sites are unchanged — they simply address a database containing exactly one campaign. - Handles are pooled with an LRU cap (64). A Map preserves insertion order, so re-inserting on read is all the LRU needs. - Campaign ids become a path segment, so they are validated against a UUID pattern rather than sanitized. A rejected id is a bug or an attack and neither should be repaired into something that opens a file. - Migrations run lazily per database on first open, so adding a campaign never requires a migration pass over every existing file. - The 18 private ensureDb() helpers no longer compute a path. withSession now derives SessionContext from the verified tenant instead of reading sessionId out of tool arguments, which are assembled from model output. The 'default' fallback is gone; unscoped requests get a sentinel that does not read like a real session id. Two boot-time bindings had to move, both of which captured one database for the process lifetime: - AuditLogger resolved its repository in the constructor. It now resolves per write, which also puts each audit row in its own campaign database. - The agent runtime bound eight repositories at startup. agent_manage and combat_manage already build it lazily from the request database, so the startup wiring is removed; provider initialization stays at boot since it is genuinely process-wide. getDb(path) remains as a test-only escape hatch and throws outside tests. That is what lets all 68 existing test files keep driving handlers directly without a tenant scope, unchanged.
📝 WalkthroughWalkthroughThe server now resolves SQLite storage per verified campaign tenant. Database handles are pooled and isolated by campaign. Request sessions and audit writes use tenant-derived context. Repository callers delegate database selection to ChangesCampaign-scoped storage
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The change moves campaigns to separate databases, but currently makes existing data unreachable without a migration path, can close a database still in use during high concurrency, and can cause one unverified subscription to remove another. Merge should wait until these data, availability, and isolation risks are addressed. Sequence Diagram(s)sequenceDiagram
participant Request
participant withSession
participant getTenant
participant getDb
participant CampaignDatabasePool
Request->>withSession: invoke handler with arguments
withSession->>getTenant: resolve verified tenant
getTenant-->>withSession: accountId, campaignId, worldId
withSession->>getDb: resolve tenant database
getDb->>CampaignDatabasePool: open or reuse campaign handle
CampaignDatabasePool-->>getDb: return database handle
getDb-->>withSession: return campaign database
withSession-->>Request: execute handler with SessionContext
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44e6255024
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (path !== undefined) { | ||
| if (process.env.NODE_ENV !== 'test') { | ||
| throw new Error( | ||
| 'getDb(path) is test-only. Production callers must use getDb(), which resolves ' + | ||
| 'the database from the verified tenant context.' | ||
| ); |
There was a problem hiding this comment.
When NODE_ENV !== 'test', numerous registered handlers still pass a path, including character_manage, world_manage, npc_manage, combat_manage, and their underlying combat handlers. Those calls now throw getDb(path) is test-only before the verified tenant can be resolved, so major tool families fail on every production invocation; convert all remaining production call sites to getDb() before enforcing this guard.
Useful? React with 👍 / 👎.
| if (overrideDb) return overrideDb; | ||
|
|
||
| return openCampaignDb(requireTenant().campaignId); |
There was a problem hiding this comment.
The default stdio transport—and the TCP, Unix-socket, and WebSocket transports—never establish runInTenant; only the HTTP transport verifies x-rpg-tenant. Consequently every storage-backed tool using the new no-argument path reaches this requireTenant() and fails, including the advertised Claude Desktop configuration that launches the server over stdio. These transports need a trusted tenant-binding mechanism, or they must be explicitly retired rather than left accepting unusable tool calls.
Useful? React with 👍 / 👎.
| const db = pool.get(oldest); | ||
| pool.delete(oldest); | ||
| try { | ||
| db?.pragma('wal_checkpoint(TRUNCATE)'); | ||
| db?.close(); |
There was a problem hiding this comment.
With more than 64 campaigns active concurrently, this closes the least-recently-acquired handle even when an in-flight request still owns repositories backed by it. For example, agent_manage invoke acquires its campaign runtime, awaits the LLM provider, and then writes through the same repositories; if 64 other campaigns open databases during that await, this code evicts and closes the first handle, causing its post-provider audit/state writes to fail. Track active leases/reference counts or otherwise avoid closing handles until their requests finish.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/index.ts`:
- Around line 221-223: Update the database selection flow around requireTenant()
and openCampaignDb() to add a migration gate for legacy shared-database rows
before routing requests to campaign files. Import existing shared data into the
relevant campaign file, or prevent campaign-file access until an external
migration has completed; preserve the overrideDb path and ensure no existing
rows become inaccessible during rollout.
- Around line 174-175: Update the database-handle lifecycle around pool.set,
evictBeyondCap, and the related lines 179-187 to track active request leases or
reference counts; only evict and close handles with no active owner, including
requests retaining a handle across await. Add a concurrency test that holds one
handle across an await while opening more than 64 other campaigns and verifies
the retained handle remains usable.
In `@tests/server/events.test.ts`:
- Around line 38-41: Update withSession and the event-subscription flow so
unverified subscriptions cannot share the sentinel sessionId "unscoped": either
reject requests without a verified tenant or assign each connection a unique
identifier. Preserve verified-tenant behavior, and add a regression test
covering two simultaneous unverified subscriptions to ensure both remain active.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4fb2b42-4831-4e08-bed5-abeeeb58881e
📒 Files selected for processing (24)
src/server/audit.tssrc/server/consolidated/agent-manage.tssrc/server/consolidated/batch-manage.tssrc/server/consolidated/corpse-manage.tssrc/server/consolidated/improvisation-manage.tssrc/server/consolidated/inventory-manage.tssrc/server/consolidated/item-manage.tssrc/server/consolidated/math-manage.tssrc/server/consolidated/narrative-manage.tssrc/server/consolidated/party-manage.tssrc/server/consolidated/perception-manage.tssrc/server/consolidated/quest-manage.tssrc/server/consolidated/scene-manage.tssrc/server/consolidated/secret-manage.tssrc/server/consolidated/session-manage.tssrc/server/consolidated/spawn-manage.tssrc/server/consolidated/strategy-manage.tssrc/server/consolidated/travel-manage.tssrc/server/consolidated/turn-manage.tssrc/server/index.tssrc/server/types.tssrc/storage/index.tstests/server/events.test.tstests/storage/per-campaign-db.test.ts
| pool.set(campaignId, db); | ||
| evictBeyondCap(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not close database handles that an active request retains.
Line 175 can evict and close a database handle after another request retained it across an await. When more than 64 campaigns interleave, the resumed request can use a closed better-sqlite3 handle and fail. Track request leases or reference counts. Evict only handles with no active owner. Add a concurrency test that holds a handle across an await while opening more than 64 other campaigns.
Also applies to: 179-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/index.ts` around lines 174 - 175, Update the database-handle
lifecycle around pool.set, evictBeyondCap, and the related lines 179-187 to
track active request leases or reference counts; only evict and close handles
with no active owner, including requests retaining a handle across await. Add a
concurrency test that holds one handle across an await while opening more than
64 other campaigns and verifies the retained handle remains usable.
| if (overrideDb) return overrideDb; | ||
|
|
||
| return openCampaignDb(requireTenant().campa 8000 ignId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add a migration gate for existing shared database data.
Line 223 routes every request to a new campaign file. Existing rows in the shared database become unavailable immediately after deployment because this code has no import or fallback path. Migrate legacy data into campaign files before serving requests, or block the rollout until an external migration completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/index.ts` around lines 221 - 223, Update the database selection
flow around requireTenant() and openCampaignDb() to add a migration gate for
legacy shared-database rows before routing requests to campaign files. Import
existing shared data into the relevant campaign file, or prevent campaign-file
access until an external migration has completed; preserve the overrideDb path
and ensure no existing rows become inaccessible during rollout.
| // 'unscoped' rather than the old 'default': this subscription was | ||
| // made outside any verified tenant, and the sentinel should not | ||
| // read like a real session id that callers could be pooled into. | ||
| params: { topic: 'world', payload: { data: 'test' }, sessionId: 'unscoped' } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'activeSubscriptions|sessionId|unscoped' src/server/events.ts tests/server/events.test.tsRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 5737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session helper and related call sites ---'
rg -n -C 12 'function withSession|const withSession|withSession\(|unscoped|verified tenant|sessionId' src tests -g '*.{ts,tsx}' | head -n 400
printf '%s\n' '--- events test setup and subscription cases ---'
sed -n '1,180p' tests/server/events.test.tsRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 32511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- events.ts imports and implementation ---'
sed -n '1,110p' src/server/events.ts
printf '%s\n' '--- all withSession definitions and imports ---'
rg -n -C 8 'withSession' src tests -g '*.{ts,tsx}' | head -n 500
printf '%s\n' '--- session context types and transport/tool dispatch ---'
rg -n -C 10 'sessionId|CallToolResult|RequestHandlerExtra|extra' src/server src -g '*.{ts,tsx}' | head -n 500Repository: Mnehmos/mnehmos.rpg.mcp
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/server/types.ts ---'
sed -n '1,90p' src/server/types.ts
printf '%s\n' '--- src/engine/pubsub.ts ---'
sed -n '1,180p' src/engine/pubsub.ts
printf '%s\n' '--- event registration lifecycle ---'
sed -n '100,145p' src/server/index.ts
sed -n '150,205p' src/server/transport/http.tsRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 7182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
types = Path("src/server/types.ts").read_text()
events = Path("src/server/events.ts").read_text()
http = Path("src/server/transport/http.ts").read_text()
assert "const UNSCOPED = 'unscoped';" in types
assert "sessionId: tenant ? `${tenant.accountId}:${tenant.campaignId}` : UNSCOPED" in types
assert "const activeSubscriptions: Map<string, Array<() => void>> = new Map();" in events
assert "activeSubscriptions.get(sessionId)" in events
assert "activeSubscriptions.set(sessionId, newSubs)" in events
assert "new PubSub" not in http
assert "sessionIdGenerator: undefined" in http
# Behavioral model of the relevant subscription/cleanup logic.
subscriptions = {"world": set()}
active = {}
def subscribe(key, callback):
subscriptions["world"].add(callback)
active[key] = [lambda: subscriptions["world"].discard(callback)]
first = lambda payload: "first"
second = lambda payload: "second"
key = "unscoped"
active.get(key, [])
subscribe(key, first)
for unsubscribe in active.get(key, []):
unsubscribe()
subscribe(key, second)
assert first not in subscriptions["world"]
assert second in subscriptions["world"]
print("unscoped requests resolve to one key; the second subscription removes the first")
print("HTTP uses stateless per-request transports while the event PubSub is shared by the server factory")
PYRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 339
Do not use one shared key for unverified subscriptions.
withSession maps every request without a verified tenant to sessionId: 'unscoped'. A second unverified subscription therefore removes the first subscription from activeSubscriptions. Reject unverified event subscriptions or assign each connection a unique identifier. Add a regression test with two unverified subscriptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/server/events.test.ts` around lines 38 - 41, Update withSession and the
event-subscription flow so unverified subscriptions cannot share the sentinel
sessionId "unscoped": either reject requests without a verified tenant or assign
each connection a unique identifier. Preserve verified-tenant behavior, and add
a regression test covering two simultaneous unverified subscriptions to ensure
both remain active.
Phase 2 of the multi-user plan. Phase 1 established a verified tenant; this consumes it. getDb() now resolves a SQLite file per campaign under RPG_DATA_DIR/campaigns/<shard>/<campaignId>.db, keyed on the campaign id from the signed tenant context. Isolation becomes physical rather than a predicate every query has to remember: a campaign own rows are the only rows in the file, so a query that forgets to scope cannot reach another tenant data. The 30 repositories and 217 call sites are unchanged — they simply address a database containing exactly one campaign. - Handles are pooled with an LRU cap (64). A Map preserves insertion order, so re-inserting on read is all the LRU needs. - Campaign ids become a path segment, so they are validated against a UUID pattern rather than sanitized. A rejected id is a bug or an attack and neither should be repaired into something that opens a file. - Migrations run lazily per database on first open, so adding a campaign never requires a migration pass over every existing file. - The 18 private ensureDb() helpers no longer compute a path. withSession now derives SessionContext from the verified tenant instead of reading sessionId out of tool arguments, which are assembled from model output. The 'default' fallback is gone; unscoped requests get a sentinel that does not read like a real session id. Two boot-time bindings had to move, both of which captured one database for the process lifetime: - AuditLogger resolved its repository in the constructor. It now resolves per write, which also puts each audit row in its own campaign database. - The agent runtime bound eight repositories at startup. agent_manage and combat_manage already build it lazily from the request database, so the startup wiring is removed; provider initialization stays at boot since it is genuinely process-wide. getDb(path) remains as a test-only escape hatch and throws outside tests. That is what lets all 68 existing test files keep driving handlers directly without a tenant scope, unchanged.
Phase 2 of the multi-user plan. Phase 1 (#82) established a verified tenant; this consumes it.
What
getDb()resolves a SQLite file per campaign underRPG_DATA_DIR/campaigns/<shard>/<campaignId>.db, keyed on the campaign id from the signed tenant context.Isolation becomes physical rather than a discipline. A campaign's rows are the only rows in the file, so a query that forgets to scope cannot reach another tenant's data. The 30 repositories and 217 call sites are unchanged — they simply address a database containing exactly one campaign.
The audit found zero repositories enforcing a tenant predicate and 34 of 48 tables with no scope column at all. Adding predicates everywhere would have meant ~292 edit points and a rule every future query must remember. This is the alternative: one resolution point, and the data isn't there to leak.
Mappreserves insertion order, so re-inserting on read is all the LRU needs.ensureDb()helpers no longer compute a path.withSessionnow derivesSessionContextfrom the verified tenant instead of readingsessionIdout of tool arguments, which are assembled from model output. The'default'fallback is gone; unscoped requests get a sentinel that doesn't read like a real session id.Two boot-time bindings had to move
Both captured a single database for the process lifetime, which cannot work per campaign:
agent_manageandcombat_managealready build it lazily from the request's database, so the startup wiring is removed. 8000 Provider initialization stays at boot since it is genuinely process-wide.Risk
This is the cutover. Existing campaign data becomes unreachable.
/app/data/rpg-mcp.dbis not migrated and becomes orphaned. This is the planned behaviour — production holds only test campaigns — but it is not reversible by rollback alone once new campaigns have been written.reference_engine_sessionsstill maps each account/campaign to reference-engine world/party/character ids that no longer exist in the fresh databases. Existing test campaigns will therefore fail with "not found" rather than cleanly disappearing. Cutover should clear those rows, or simply create new campaigns and abandon the old ones.RPG_MCP_DB_PATHbecomes dead configuration and should be unset to avoid implying the engine still opens one shared file.getDb(path)now throws outside tests. Verified no production caller passes a path after this change.Rollback: reverting restores the single-database behaviour, but any campaign created after deploy lives only in a per-campaign file and would not be visible again.
Acceptance criteria
SELECT * FROM characters.getDb()with no verified tenant throws rather than defaulting.awaitpoints.Verification
npm test— 2,343 passed / 7 skipped, no failures.npx tsc --noEmit— exit 0.npm run build— clean.11 new tests in
tests/storage/per-campaign-db.test.ts. The isolation test deliberately callsfindAll(), which issues an unscopedSELECT * FROM characters— that is the point: the query cannot leak because the row is not in the file.Not covered: a live end-to-end turn against the deployed engine. That is the deploy-time check.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests