feat(community): community.export() — embedded (#79)#100
Open
Rinse12 wants to merge 14 commits into
Open
Conversation
Implements the embedded slice of issue #79: `community.export()` produces a consistent sqlite snapshot of a LocalCommunity at `<pkcDataPath>/exports/<exportId>.sqlite`, exposed through `community.exports` and the `exportschange` event. Per Esteban's feedback on the design thread, the public CommunityExportRecord exposes only `progress` (0..1), `error`, and the terminal `size`/`sha256`/`url` fields. Consumers infer state: complete = progress===1, failed/cancelled = error!==undefined. Internal pagesCopied/totalPages/startedAt/completedAt and the explicit state enum are intentionally omitted; they can be added back compatibly if a finer-grained state surface is ever needed. What's in this PR: - Spec doc: src/rpc/EXPORT_COMMUNITY_SPEC.md - New zod schemas + types: ExportCommunityUserOptions, CommunityExportRecord - backupCommunityDb helper: better-sqlite3 backup with AbortSignal-aware progress callback, atomic .partial→final rename, sha256 hashing, private-key scrub of the signer record when includePrivateKey is false - LocalCommunity: _exports / _activeExports / _exportQueue state, export(), exports getter, _cancelExport(), persistence to a new STORAGE_KEYS.EXPORTS keyv entry with prune-on-load for files that vanished out of band - RemoteCommunity base: read-only stubs (export() rejects with ERR_COMMUNITY_NOT_LOCAL; exports returns []) - pkc.destroy() aborts in-flight exports and awaits their unwind before tearing down storage, matching the spec - 10 vitest cases covering happy path, includePrivateKey on/off, exportPath override, exportschange emissions, AbortSignal mid-flight, pre-aborted signal, concurrent exports of the same community, ERR_COMMUNITY_NOT_LOCAL, and pkc.destroy() teardown Follow-up (tracked on #79): the RPC wire protocol (exportCommunity, exportsSubscribe, cancelExport), the HTTP /exports/<id> download endpoint, 24h orphan sweep, and tests under test/node-and-browser/rpc/.
📝 WalkthroughWalkthroughImplements community database export: adds Zod schemas, event/type and error/storage-key declarations, a base RemoteCommunity export API that rejects for non-local communities, Node SQLite backup utilities with cancellation and hashing, LocalCommunity embedded export flow with persistent progress and cancellation, RPC client/server export plumbing and HTTP download endpoint, lifecycle wiring, docs, and comprehensive tests. ChangesCommunity Export Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/community/schema.ts (1)
363-388: ⚡ Quick winPlease confirm the new runtime field is reflected in reserved-field handling.
exportsis now a runtime-only community field, but this reserved-field list wasn’t updated. Please confirm whether it should be added here to avoid accidental leakage into signed/public payload paths.As per coding guidelines, "Every time you add a runtime-only field, ask whether it should also be added to the corresponding reserved-field list before you finish the change".
🤖 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/community/schema.ts` around lines 363 - 388, The reserved-field list CommunityIpfsReservedFields is missing the new runtime-only field "exports", so add "exports" to the first array passed to remeda.difference so it will be excluded from signed/public payloads (keep using remeda.keys.strict(CommunityIpfsSchema.shape) for the schema comparison); also verify CommunityIpfsSchema does not include "exports" as a persisted field and update the TODO typing comment if desired to reflect the enum of reserved props.
🤖 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/community/schema.ts`:
- Around line 344-359: The schema CommunityExportRecordSchema currently leaves
size, sha256 and url optional even though the comment requires they be present
when progress === 1; update CommunityExportRecordSchema to enforce that
invariant by adding a refinement (z.refine or .superRefine) on the object that
checks the progress field and if progress === 1 then ensures size is defined
(and non-null), sha256 matches the hex regex, and url is defined; keep the
existing individual validators (size int/nonnegative, sha256 regex) but make the
overall validator reject objects where progress === 1 and any of size/sha256/url
is missing or invalid.
In `@src/rpc/EXPORT_COMMUNITY_SPEC.md`:
- Around line 109-140: Summary: The document reads as if RPC methods, the HTTP
download endpoint, and the 24h orphan sweep are implemented; they must be marked
as planned/not-yet-implemented. Edit EXPORT_COMMUNITY_SPEC.md to explicitly
prefix or annotate the `exportCommunity`, `exportsSubscribe`, and `cancelExport`
RPC descriptions (and any mentions of the HTTP `/exports/<exportId>` download
endpoint and the "24h orphan sweep") with a clear "Planned / Not yet
implemented" notice; clarify that AbortSignal routing, server behavior on
disconnect, and retention/orphan-sweep semantics are future work and not part of
the embedded-export scope; ensure `community.exports` and
`_findCommunityAddress` descriptions remain but are described as intended design
only. Also apply the same wording change to the later referenced section (the
content around lines noted in the review) so consumers and tests cannot assume
these behaviors are implemented.
- Around line 159-160: Steps 5–6 in EXPORT_COMMUNITY_SPEC.md are out of order:
you must rename the partial file to the final path before computing the SHA256
of the finalized file. Update the doc so that fs.rename(destPath + ".partial",
destPath) is listed before "Compute sha256 of the finalized file" (or explicitly
state that the hash is computed against destPath after the atomic rename),
referencing the filenames destPath and destPath + ".partial" and the fs.rename
operation to make the sequence unambiguous.
In `@src/runtime/node/community/local-community.ts`:
- Around line 388-390: The getter override get exports currently returns a
shallow-cloned array but still exposes live CommunityExportRecord objects from
this._exports; update the exports getter to return deep-cloned records (e.g.,
map over this._exports and clone each record) so callers cannot mutate internal
state, and reuse the same cloning helper used for preparing the exportschange
payloads to ensure consistent cloning logic across the class (reference the
override get exports, this._exports, CommunityExportRecord type, and the
exportschange payload helper).
- Around line 75-82: The import list in local-community.ts currently brings
InternalExportHandle as a value import alongside cancelExportEmbedded,
exportCommunityEmbedded, and loadAndPruneExportsFromKeyv; change it so
InternalExportHandle is imported using a type-only import (import type {
InternalExportHandle } from "./local-community/export.js") while keeping the
other symbols as regular imports to avoid runtime import hazards under TS ESM
emit settings.
In `@src/runtime/node/community/local-community/export.ts`:
- Around line 116-117: The export currently allows options.exportPath to resolve
to the live DB and later overwrites it; after computing destPath (from
options.exportPath or defaultExportPathFor(community, exportId)) add a
protection that resolves and compares destPath against
sourceDbPathFor(community) and throw/reject if they are the same (or if destPath
is a path inside the live DB directory, if applicable) to prevent clobbering the
active database; include a clear error message mentioning exportPath and that
the target equals the live DB, and perform this check before any rename or write
operations and before using includePrivateKey to continue.
- Around line 89-98: Replace the loose catch typing by using catch (err:
unknown) and narrow the error before using .message: when handling the abort
branch preserve the existing BackupAbortError check and opts.signal.aborted, but
derive the message with something like const msg = err instanceof Error ?
err.message : String(err) and use msg in the updateRecord call for the
"ERR_EXPORT_CANCELLED" case; in the non-abort branch log the raw err (or msg)
via log.error and set the updateRecord error message to the same narrowed msg
for "ERR_EXPORT_BACKUP_FAILED"; update references in this block around
updateRecord, BackupAbortError, exportId, community, and log.error accordingly.
In `@src/runtime/node/util.ts`:
- Around line 378-385: After the backup finishes this block ignores the provided
AbortSignal so cancellation during scrub/hash/rename still reports success;
update the code around includePrivateKey, scrubPrivateKeyFromBackup,
statAndHashFile, and fsPromises.rename to honor cancellation by checking
signal.aborted (or throwing an AbortError) before each await and immediately
after each awaited call, or pass the signal into
scrubPrivateKeyFromBackup/statAndHashFile if they accept it, and ensure any
in-progress rename is aborted/rolled back and the function rethrows an abort so
pkc.destroy() sees the cancellation.
- Around line 406-410: The code currently casts JSON.parse(row.value) to {
value: Record<string, any>; expires?: number | null } and then mutates
parsed.value.signer; replace that any-escape with a safe refinement: parse into
unknown, verify parsed is an object with a value property that is an object, and
that value.signer (if present) is an object with optional privateKey?: string
and ipfsKey?: Uint8Array (or nullable variants); only after these runtime type
checks mutate signer.privateKey = undefined and signer.ipfsKey = undefined.
Update the variable around JSON.parse(...) (the parsed variable inside
scrubPrivateKeyFromBackup) to use unknown + type guards before touching signer
so typing remains strict and safe.
In `@test/node/community/export.test.ts`:
- Line 77: Several tests use the helper itSkipIfRpc (and describeSkipIfRpc)
without the required explanatory comment; for each occurrence (e.g., the
itSkipIfRpc call shown, plus the ones at the other noted locations) add a short
single-line comment immediately above the call stating why RPC cannot run that
test (for example: "// RPC cannot access local filesystem to validate sha256" or
a similarly specific rationale). Make sure the comment is directly above the
itSkipIfRpc/describeSkipIfRpc invocation and references the reason
(network/FS/permission/DB constraints) so it satisfies the coding guideline.
- Line 115: The test currently casts signer as `any` when parsing row!.value;
replace that with a proper signer type by either importing the existing
Signer/CommunitySigner interface used by production fixtures or by defining a
local narrow interface (e.g., { id: string; name?: string; publicKey?: string }
matching the fixture shape) and change the parse to JSON.parse(row!.value) as {
value: { signer: Signer } } (or your local interface), then update subsequent
assertions to use the typed signer fields instead of relying on `any`.
---
Nitpick comments:
In `@src/community/schema.ts`:
- Around line 363-388: The reserved-field list CommunityIpfsReservedFields is
missing the new runtime-only field "exports", so add "exports" to the first
array passed to remeda.difference so it will be excluded from signed/public
payloads (keep using remeda.keys.strict(CommunityIpfsSchema.shape) for the
schema comparison); also verify CommunityIpfsSchema does not include "exports"
as a persisted field and update the TODO typing comment if desired to reflect
the enum of reserved props.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9904ea84-f4e2-4d29-bc2b-c40a7929b48d
📒 Files selected for processing (12)
src/community/remote-community.tssrc/community/schema.tssrc/community/types.tssrc/constants.tssrc/errors.tssrc/pkc/pkc.tssrc/rpc/EXPORT_COMMUNITY_SPEC.mdsrc/runtime/node/community/local-community.tssrc/runtime/node/community/local-community/export.tssrc/runtime/node/community/local-community/lifecycle.tssrc/runtime/node/util.tstest/node/community/export.test.ts
Comment on lines
+378
to
+385
| if (!includePrivateKey) { | ||
| await scrubPrivateKeyFromBackup(partialPath); | ||
| } | ||
|
|
||
| const { size, sha256 } = await statAndHashFile(partialPath); | ||
|
|
||
| await fsPromises.rename(partialPath, destPath); | ||
|
|
There was a problem hiding this comment.
Honor cancellation during scrub/hash/rename.
Once the SQLite backup finishes, this code no longer checks signal.aborted, so a cancel that arrives during scrubbing or hashing still completes as a successful export. That breaks the AbortSignal contract and makes pkc.destroy() race with long post-processing on large DBs.
Suggested fix
if (!includePrivateKey) {
await scrubPrivateKeyFromBackup(partialPath);
+ if (signal?.aborted) throw new BackupAbortError();
}
const { size, sha256 } = await statAndHashFile(partialPath);
+ if (signal?.aborted) throw new BackupAbortError();
await fsPromises.rename(partialPath, destPath);
+ if (signal?.aborted) throw new BackupAbortError();
return { size, sha256 };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!includePrivateKey) { | |
| await scrubPrivateKeyFromBackup(partialPath); | |
| } | |
| const { size, sha256 } = await statAndHashFile(partialPath); | |
| await fsPromises.rename(partialPath, destPath); | |
| if (!includePrivateKey) { | |
| await scrubPrivateKeyFromBackup(partialPath); | |
| if (signal?.aborted) throw new BackupAbortError(); | |
| } | |
| const { size, sha256 } = await statAndHashFile(partialPath); | |
| if (signal?.aborted) throw new BackupAbortError(); | |
| await fsPromises.rename(partialPath, destPath); | |
| if (signal?.aborted) throw new BackupAbortError(); |
🤖 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/runtime/node/util.ts` around lines 378 - 385, After the backup finishes
this block ignores the provided AbortSignal so cancellation during
scrub/hash/rename still reports success; update the code around
includePrivateKey, scrubPrivateKeyFromBackup, statAndHashFile, and
fsPromises.rename to honor cancellation by checking signal.aborted (or throwing
an AbortError) before each await and immediately after each awaited call, or
pass the signal into scrubPrivateKeyFromBackup/statAndHashFile if they accept
it, and ensure any in-progress rename is aborted/rolled back and the function
rethrows an abort so pkc.destroy() sees the cancellation.
| await pkc.destroy(); | ||
| }); | ||
|
|
||
| itSkipIfRpc("happy path: file is on disk, sha256 matches, sqlite is readable", async () => { |
There was a problem hiding this comment.
Add explicit rationale comments above each itSkipIfRpc usage.
Each skipped RPC test needs a short “why RPC cannot run this” comment directly above it. Right now that requirement isn’t met.
As per coding guidelines, "When using describeSkipIfRpc, itSkipIfRpc, or otherwise skipping tests for RPC, you MUST add a comment above explaining why the test cannot run under RPC".
Also applies to: 106-106, 126-126, 142-142, 155-155, 172-172, 188-188, 196-196, 209-209, 230-230
🤖 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 `@test/node/community/export.test.ts` at line 77, Several tests use the helper
itSkipIfRpc (and describeSkipIfRpc) without the required explanatory comment;
for each occurrence (e.g., the itSkipIfRpc call shown, plus the ones at the
other noted locations) add a short single-line comment immediately above the
call stating why RPC cannot run that test (for example: "// RPC cannot access
local filesystem to validate sha256" or a similarly specific rationale). Make
sure the comment is directly above the itSkipIfRpc/describeSkipIfRpc invocation
and references the reason (network/FS/permission/DB constraints) so it satisfies
the coding guideline.
…dCommunityOrDb
A fresh LocalCommunity instance now sees prior exports in community.exports as
soon as pkc.createCommunity({ address }) returns, mirroring how internal state
is loaded. Two branches:
- Sibling started instance in the same PKC: snapshot _exports from it.
- DB load path: call _loadExportsFromKeyv() before destroying the connection.
Test covers both: a same-PKC sibling and a brand-new PKC instance pointed at
the same dataPath both see the previously-completed export's progress, url,
and sha256.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/runtime/node/community/local-community/db-state.ts (1)
151-154: ⚡ Quick winDeep-clone export records for true snapshot semantics.
This currently snapshots only the array, not the record objects. If a record is mutated in place later, state can still leak across instances despite the non-propagation intent.
Suggested change
- community._exports = [...startedCommunity._exports]; + community._exports = startedCommunity._exports.map((record) => ({ + ...record, + ...(record.error ? { error: { ...record.error } } : {}) + }));🤖 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/runtime/node/community/local-community/db-state.ts` around lines 151 - 154, The current snapshot copies only the array reference for community._exports from startedCommunity._exports which still allows in-place mutations of export record objects to leak across instances; replace the shallow copy with a deep clone of each export record so community._exports = startedCommunity._exports.map(record => deep-clone(record)); use a reliable cloning approach available in the runtime (e.g., structuredClone if supported or a JSON deep-clone fallback) and ensure this change is applied where community._exports is assigned so each record object is a true snapshot.
🤖 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.
Nitpick comments:
In `@src/runtime/node/community/local-community/db-state.ts`:
- Around line 151-154: The current snapshot copies only the array reference for
community._exports from startedCommunity._exports which still allows in-place
mutations of export record objects to leak across instances; replace the shallow
copy with a deep clone of each export record so community._exports =
startedCommunity._exports.map(record => deep-clone(record)); use a reliable
cloning approach available in the runtime (e.g., structuredClone if supported or
a JSON deep-clone fallback) and ensure this change is applied where
community._exports is assigned so each record object is a true snapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 524720cb-d780-4d75-b8fa-b796eefc1395
📒 Files selected for processing (2)
src/runtime/node/community/local-community/db-state.tstest/node/community/export.test.ts
- Reject exportPath that resolves to the live community DB (new ERR_EXPORT_PATH_TARGETS_LIVE_DB) to prevent clobbering it - Honor AbortSignal during post-backup scrub/hash/rename so pkc.destroy() cannot race with long post-processing - Enforce size/sha256/url presence when progress === 1 via CommunityExportRecordSchema.superRefine - Add 'exports' to CommunityIpfsReservedFields - Deep-clone records returned via community.exports and the exportschange payload (shared helper cloneExportRecord) - Replace Record<string, any> in scrubPrivateKeyFromBackup with a typed runtime guard; document scrub scope (community signer only — pseudonymityAliases.aliasPrivateKey intentionally kept) - Narrow catch (err: unknown) in runExportTask; switch InternalExportHandle to import type - Add per-call RPC-skip rationales and remove `as any` from the signer parse in export.test.ts - Update EXPORT_COMMUNITY_SPEC.md: status table, mark deferred RPC/HTTP/sweep sections as Planned, reorder backup steps so rename precedes hash, document new error code and snapshot semantics
…lers Registers the three wire methods on PKCWsServer with their zod schemas and parsers. Server-side records carry file:// URLs; the wire format converts them to a relative /exports/<exportId> URL so the RPC client can absolutize against the WS server's HTTP origin. The exportsSubscribe handler emits the current exports snapshot as the initial notification and pipes the LocalCommunity exportschange event into push notifications thereafter. Tearing down the subscription does not cancel any in-flight exports — exports outlive client connections. Adds an allowPrivateKeyExport flag (default true) so public-RPC operators can refuse includePrivateKey: true requests. Spec: src/rpc/EXPORT_COMMUNITY_SPEC.md Issue: #79
PKCWsServer now always operates on an explicit http.Server (creating one when only port is provided) so the WebSocket upgrade and the HTTP download route can live on the same port. A successful download deletes the file and prunes the record via the new LocalCommunity._deleteExport helper, matching the spec's post-download cleanup semantics. Client aborts and stream errors leave the record intact so the consumer can retry. Adds the deleteExportRecord helper alongside the existing cancel/load helpers in local-community/export.ts so the deletion path stays adjacent to the other lifecycle transitions. Issue: #79
PKCRpcClient gains exportCommunity / exportsSubscribe / cancelExport methods plus an rpcHttpOrigin getter that derives http[s]://host from the WS URL so the client can absolutize the relative `/exports/<exportId>` URLs that the server emits. RpcLocalCommunity now overrides export() and the exports getter. The subscription is attached eagerly during pkc.createCommunity (before it resolves), so consumers see prior exports — including ones still running from an earlier client session — synchronously after createCommunity. This is what lets a fresh client survive its predecessor's disconnect: the server keeps the export going, and the new client's initial exportschange notification reveals the in-flight record. AbortSignal cancellation routes through cancelExport on the wire, with the listener detached automatically once the record reaches a terminal state. Issue: #79
Deletes any *.sqlite files under <pkcDataPath>/exports/ whose mtime is over 24h old. Records pointing to the deleted files are pruned lazily the next time the community loads via loadAndPruneExportsFromKeyv. Flips the spec status table for the RPC surfaces shipped in this PR. Issue: #79
Wraps the export tests with getAvailablePKCConfigsToTestAgainst so the same 8 portable cases run under local-kubo-rpc and remote-pkc-rpc. The new materializeExport helper hides the file:// vs http:// URL difference by downloading HTTP-served exports to a local temp path before the sha256 and sqlite-open assertions. Two tests stay embedded-only by design: - exportPath option (RPC variant lives in the RPC-only suite below as a sync rejection check) - pkc.destroy() cancels in-flight exports (RPC client disconnect must not cancel server exports, per spec — disconnect-resume is the RPC variant) The RPC-only suite adds: exportPath sync rejection, HTTP 404 for unknown exportId, post-download server-side cleanup (file deleted, record pruned, second fetch returns 404), and the load-bearing disconnect-resume scenario (client A starts an export, disconnects mid-flight, client B reconnects and sees the record un-cancelled and finishing to progress=1). Issue: #79
…he cache Factors the 8 portable test bodies into a defineExportTests(getCtx) helper, then calls it from three setups: embedded started, RPC started, RPC non-started (new). The non-started suite is what keeps PKCWsServer's _exportCommunityInstances cache honest — without the cache, exportCommunity and the eager exportsSubscribe would call pkc.createCommunity separately and land on different LocalCommunity objects (pkc.ts:749 builds a new one each call), so the export's exportschange would never reach the subscription's listener. Multiple polling assertions would then time out, giving us a loud regression signal if the cache breaks. Bootstrap is necessary because publishing a post requires the community to be running; we start once via a throwaway client, publish, stop, then disconnect so the LocalCommunity instance is gone from _startedCommunities. Issue: #79
The old order (started → cache) broke the typical client flow: 1. pkc.createCommunity() — server makes LocalCommunity #1 2. _attachExportsSubscription — community not yet started, falls through to pkc.createCommunity → fresh #2, cached, listener on #2 3. community.start() — server's startCommunity → fresh #3 in _startedCommunities 4. community.export() — findStartedCommunity returns #3, export runs on #3, exportschange fires on #3 — but the listener is on #2 Symptom: the test server log shows "Export complete for community ... exportId ..." while the client's waitForCompleteRecord times out at 30s. Cache-first ordering anchors every export-related handler on the same LocalCommunity for the address: once the first call resolves, subsequent exportCommunity / exportsSubscribe / cancelExport calls all reuse it. Issue: #79
Adds an RPC-only complement to the existing embedded RemoteCommunity rejection test. Asks the daemon for a fresh signer's address that's never been a community on the server, so pkc-with-rpc-client.ts returns an RpcRemoteCommunity (which doesn't override export() and inherits the base ERR_COMMUNITY_NOT_LOCAL throw). Sanity-checks the returned object is not an RpcLocalCommunity so a future change to the create-community flow doesn't quietly turn the test into a no-op. Issue: #79
The waitForCompleteRecord / waitForRecord helpers polled community.exports every 50ms, even though the export pipeline already emits exportschange for every transition. Reuse resolveWhenConditionIsTrue on that event so each helper attaches a single listener and resolves on the next emission, racing against the timeoutMs budget. Call sites are updated to the object-parameter form per AGENTS.md.
updateInstancePropsWithStartedCommunityOrDb used to snapshot _exports from the in-memory started instance whenever findStartedCommunity or findCommunityInRegistry(processStartedCommunities, ...) returned a hit. That makes the new community's exports list depend on whichever LocalCommunity instance happens to be tracked process-wide, which can be stale or cross-PKC. Read from the keyv-backed DB instead — it is the canonical persisted source and is current after every export record mutation (persistExports is awaited inside updateRecord). Open the DB read-write rather than read-only: a read-only connection under SQLite WAL silently misses writes committed by a still-open writer on the same file, which is exactly the case for the same-PKC sibling path.
…resh-PKC reload after destroy
test.configs.pkc.test.ts: a new `it` documenting that PKC({
pkcRpcClientsOptions, dataPath }) should retain both options. Today
PKC's constructor drops dataPath when pkcRpcClientsOptions is set, so
the assertion currently fails and pins down the contract.
export.test.ts: a new `itSkipIfRpc` that asserts a fresh PKC on the
same dataPath sees the terminal-error record persisted by an in-flight
export that was cancelled by pkc.destroy(). Plus inline TODOs flagging
that we should also verify exportschange event fields explicitly
rather than only polling community.exports.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/node/community/local-community/export.ts (1)
121-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check
options.signalafter wiring the listener.Line 122 only covers signals that were already aborted before
exportCommunityEmbedded()started. If the signal aborts between that check and this listener setup, the event is missed and the export keeps running.🩹 Minimal fix
if (options.signal) { const userSignal = options.signal; const onAbort = () => controller.abort(userSignal.reason); - userSignal.addEventListener("abort", onAbort, { once: true }); - detachUserSignal = () => userSignal.removeEventListener("abort", onAbort); + if (userSignal.aborted) onAbort(); + else { + userSignal.addEventListener("abort", onAbort, { once: true }); + detachUserSignal = () => userSignal.removeEventListener("abort", onAbort); + } }Also applies to: 149-154
🤖 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/runtime/node/community/local-community/export.ts` around lines 121 - 125, The pre-listener abort check in exportCommunityEmbedded is racy: after you attach the options.signal.addEventListener("abort", ...) you must immediately re-check options.signal.aborted and throw the same error if set (use the same logic: const reason = (options.signal as AbortSignal).reason; throw reason ?? new DOMException("The operation was aborted.", "AbortError");). Apply that exact post-listener re-check in both places where you wired the listener (the block inside exportCommunityEmbedded and the similar block around lines 149-154) so an abort that happens between the initial check and listener registration is handled; keep the existing listener cleanup logic intact.src/community/schema.ts (1)
327-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten
ExportCommunityUserOptionsSchema.signalto validate requiredAbortSignalAPIs
ExportCommunityUserOptionsSchema.signalonly checks for anabortedproperty, but both export flows useaddEventListener("abort", ...)andremoveEventListener(...)onoptions.signal, so invalid objects (e.g.{ aborted: false }) can pass schema validation and fail at runtime.🩹 Minimal tightening
export const ExportCommunityUserOptionsSchema = z .object({ includePrivateKey: z.boolean().optional(), exportPath: z.string().min(1).optional(), - signal: z.custom<AbortSignal>((v) => v === undefined || (typeof v === "object" && v !== null && "aborted" in v)).optional() + signal: z + .custom<AbortSignal>( + (v) => + v === undefined || + (typeof v === "object" && + v !== null && + "aborted" in v && + typeof (v as Partial<AbortSignal>).addEventListener === "function" && + typeof (v as Partial<AbortSignal>).removeEventListener === "function") + ) + .optional() }) .strict();🤖 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/community/schema.ts` around lines 327 - 333, The schema ExportCommunityUserOptionsSchema currently validates signal only by presence of "aborted" which allows plain objects; update the z.custom<AbortSignal> validator for the signal field to ensure it is either undefined or a real AbortSignal by checking that the value is an object and has the "aborted" boolean property plus callable addEventListener and removeEventListener methods (e.g. typeof v.addEventListener === "function" && typeof v.removeEventListener === "function"), so callers using addEventListener("abort", ...) and removeEventListener(...) won't fail at runtime.
🤖 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/rpc/EXPORT_COMMUNITY_SPEC.md`:
- Around line 85-87: Several places in the EXPORT_COMMUNITY_SPEC (references:
the "RPC methods", "HTTP download", "retention behavior" sections and the
validation/error examples that mention includePrivateKey and exportPath)
disagree about whether features are "shipped" or "planned"; pick the canonical
status (e.g., mark these surfaces as shipped) and make all occurrences
consistent: update the status phrases and any parenthetical notes like "Planned
— depends on RPC server/client" to the chosen status, remove or revise
conflicting examples in the validation/error sections (including checks that
call out exportPath/exportPath-resolves-to-live-db and includePrivateKey
disallowed scenarios) so they align with the chosen model, and ensure any
downstream sections that describe behavior (RPC, HTTP download, retention, and
validation) use the same terminology and error expectations throughout.
In `@src/rpc/src/index.ts`:
- Around line 177-187: The request listener currently runs for every request and
causes _handleExportsHttpRequest to send 404s for non-/exports routes; restrict
handling to only export paths (or bail without writing response). Update the
request handler attached to this._httpServer so it first checks the request path
(e.g., req.url or parsed pathname) and only calls
this._handleExportsHttpRequest(req, res) when the path matches the export route
(e.g., startsWith "/exports/" or exact "/exports"); otherwise return immediately
(do not set status or end the response). You can alternatively add the same
early-return guard at the top of _handleExportsHttpRequest to avoid touching
other code paths.
- Around line 1662-1673: The handler that resolves /exports/<id> only searches
_exportLoadedCommunities() so exports persisted in KeyV but whose LocalCommunity
isn't started are invisible after restart; update _findCommunityOwningExport
(and the HTTP export handler logic that calls it) to also query the persistent
export records store (the KeyV-backed export persistence used for completed
CommunityExportRecord entries) when a record isn't found in
_exportLoadedCommunities(), and if found either hydrate/instantiate the
corresponding LocalCommunity (using the same initialization used by
_exportCommunityInstances) or return the persisted CommunityExportRecord
directly so the URL remains downloadable after a daemon restart.
In `@test/node-and-browser/pkc/test.configs.pkc.test.ts`:
- Line 5: The failing test imports the Node-only symbol getDefaultDataPath from
the runtime/node module, which makes the file depend on the Node build and
breaks browser runs; either move the test case that uses getDefaultDataPath out
of the shared test/node-and-browser suite into the node-only suite (e.g.,
test/node) or remove the static runtime/node import and instead gate a dynamic
Node-only import/require inside the test body (e.g., if (typeof process !==
"undefined" && process.versions?.node) { const { getDefaultDataPath } = await
import(".../runtime/node/util.js"); ... }) so the module is only loaded when
running in Node. Ensure references to getDefaultDataPath are updated
accordingly.
---
Outside diff comments:
In `@src/community/schema.ts`:
- Around line 327-333: The schema ExportCommunityUserOptionsSchema currently
validates signal only by presence of "aborted" which allows plain objects;
update the z.custom<AbortSignal> validator for the signal field to ensure it is
either undefined or a real AbortSignal by checking that the value is an object
and has the "aborted" boolean property plus callable addEventListener and
removeEventListener methods (e.g. typeof v.addEventListener === "function" &&
typeof v.removeEventListener === "function"), so callers using
addEventListener("abort", ...) and removeEventListener(...) won't fail at
runtime.
In `@src/runtime/node/community/local-community/export.ts`:
- Around line 121-125: The pre-listener abort check in exportCommunityEmbedded
is racy: after you attach the options.signal.addEventListener("abort", ...) you
must immediately re-check options.signal.aborted and throw the same error if set
(use the same logic: const reason = (options.signal as AbortSignal).reason;
throw reason ?? new DOMException("The operation was aborted.", "AbortError");).
Apply that exact post-listener re-check in both places where you wired the
listener (the block inside exportCommunityEmbedded and the similar block around
lines 149-154) so an abort that happens between the initial check and listener
registration is handled; keep the existing listener cleanup logic intact.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d48d558-fbea-4467-b6c0-1dd3e3213ced
📒 Files selected for processing (17)
src/clients/rpc-client/pkc-rpc-client.tssrc/clients/rpc-client/rpc-schema-util.tssrc/clients/rpc-client/schema.tssrc/clients/rpc-client/types.tssrc/community/rpc-local-community.tssrc/community/schema.tssrc/errors.tssrc/pkc/pkc-with-rpc-client.tssrc/rpc/EXPORT_COMMUNITY_SPEC.mdsrc/rpc/src/index.tssrc/rpc/src/schema.tssrc/runtime/node/community/local-community.tssrc/runtime/node/community/local-community/db-state.tssrc/runtime/node/community/local-community/export.tssrc/runtime/node/util.tstest/node-and-browser/pkc/test.configs.pkc.test.tstest/node/community/export.test.ts
Comment on lines
+85
to
+87
| - `includePrivateKey: true` but RPC server policy disallows it. *(Planned — depends on RPC server.)* | ||
| - `exportPath` provided but caller is using an RPC client. *(Planned — depends on RPC client.)* | ||
| - `exportPath` resolves to the live community database file (would clobber it). |
There was a problem hiding this comment.
Unify shipped-vs-planned status statements across this spec.
Line 15–Line 20 says RPC methods, HTTP download, and retention behavior are shipped, but Line 128–Line 130 and later sections still describe those same surfaces as planned/deferred. The same conflict appears in validation/error sections (for example Line 85–Line 87 and Line 241–Line 243). Please align these sections to one consistent status model to avoid contract drift.
Also applies to: 112-113, 128-130, 200-205, 216-249
🤖 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/rpc/EXPORT_COMMUNITY_SPEC.md` around lines 85 - 87, Several places in the
EXPORT_COMMUNITY_SPEC (references: the "RPC methods", "HTTP download",
"retention behavior" sections and the validation/error examples that mention
includePrivateKey and exportPath) disagree about whether features are "shipped"
or "planned"; pick the canonical status (e.g., mark these surfaces as shipped)
and make all occurrences consistent: update the status phrases and any
parenthetical notes like "Planned — depends on RPC server/client" to the chosen
status, remove or revise conflicting examples in the validation/error sections
(including checks that call out exportPath/exportPath-resolves-to-live-db and
includePrivateKey disallowed scenarios) so they align with the chosen model, and
ensure any downstream sections that describe behavior (RPC, HTTP download,
retention, and validation) use the same terminology and error expectations
throughout.
Comment on lines
+177
to
+187
| this._httpServer.on("request", (req, res) => { | ||
| // rpc-websockets does not register request listeners — any HTTP request reaching | ||
| // this http.Server is ours to handle. Currently the only route is GET /exports/<id>. | ||
| this._handleExportsHttpRequest(req, res).catch((e) => { | ||
| log.error("Unhandled error in /exports HTTP handler", e); | ||
| if (!res.headersSent) { | ||
| res.statusCode = 500; | ||
| res.end(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Don't 404 unrelated routes on caller-provided servers.
When server is supplied, this listener still runs for every HTTP request, and _handleExportsHttpRequest() sends a 404 for any non-export path. On a shared HTTP(S) server that can hijack unrelated routes or double-write the response.
🩹 Minimal fix
- this._httpServer.on("request", (req, res) => {
+ this._httpServer.on("request", (req, res) => {
+ const pathname = new URL(req.url ?? "/", "http://localhost").pathname;
+ if (server && !pathname.startsWith("/exports/")) return;
// rpc-websockets does not register request listeners — any HTTP request reaching
// this http.Server is ours to handle. Currently the only route is GET /exports/<id>.
this._handleExportsHttpRequest(req, res).catch((e) => {Also applies to: 1709-1714
🤖 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/rpc/src/index.ts` around lines 177 - 187, The request listener currently
runs for every request and causes _handleExportsHttpRequest to send 404s for
non-/exports routes; restrict handling to only export paths (or bail without
writing response). Update the request handler attached to this._httpServer so it
first checks the request path (e.g., req.url or parsed pathname) and only calls
this._handleExportsHttpRequest(req, res) when the path matches the export route
(e.g., startsWith "/exports/" or exact "/exports"); otherwise return immediately
(do not set status or end the response). You can alternatively add the same
early-return guard at the top of _handleExportsHttpRequest to avoid touching
other code paths.
Comment on lines
+1662
to
+1673
| private *_exportLoadedCommunities(): IterableIterator<LocalCommunity> { | ||
| for (const value of Object.values(this._startedCommunities)) if (value instanceof LocalCommunity) yield value; | ||
| for (const value of this._exportCommunityInstances.values()) yield value; | ||
| } | ||
|
|
||
| private _findCommunityOwningExport(exportId: string): { community: LocalCommunity; record: CommunityExportRecord } | undefined { | ||
| for (const community of this._exportLoadedCommunities()) { | ||
| const record = community._exports.find((r) => r.exportId === exportId); | ||
| if (record) return { community, record }; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Persisted export URLs become undownloadable after a restart.
The HTTP handler only looks through _exportLoadedCommunities(), which is just started communities plus _exportCommunityInstances. Completed export records survive in KeyV, but after a daemon restart none of those communities are loaded yet, so a previously valid /exports/<id> URL returns 404 until some other export RPC call happens to hydrate that community first.
Also applies to: 1722-1728
🤖 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/rpc/src/index.ts` around lines 1662 - 1673, The handler that resolves
/exports/<id> only searches _exportLoadedCommunities() so exports persisted in
KeyV but whose LocalCommunity isn't started are invisible after restart; update
_findCommunityOwningExport (and the HTTP export handler logic that calls it) to
also query the persistent export records store (the KeyV-backed export
persistence used for completed CommunityExportRecord entries) when a record
isn't found in _exportLoadedCommunities(), and if found either
hydrate/instantiate the corresponding LocalCommunity (using the same
initialization used by _exportCommunityInstances) or return the persisted
CommunityExportRecord directly so the URL remains downloadable after a daemon
restart.
| import PKCFactory from "../../../dist/node/index.js"; | ||
| import { getAvailablePKCConfigsToTestAgainst, isRpcFlagOn, isRunningInBrowser, mockPKC } from "../../../dist/node/test/test-util.js"; | ||
| import { itIfRpc } from "../../helpers/conditional-tests.js"; | ||
| import { getDefaultDataPath } from "../../../dist/node/runtime/node/util.js"; |
There was a problem hiding this comment.
Move this case out of test/node-and-browser.
getDefaultDataPath comes from a node-only runtime module, so this file now hard-depends on the node build before the test body runs. That makes the new case unsafe for the browser leg of this suite. Either move it under test/node/ or avoid the runtime/node import here.
Also applies to: 151-170
🤖 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 `@test/node-and-browser/pkc/test.configs.pkc.test.ts` at line 5, The failing
test imports the Node-only symbol getDefaultDataPath from the runtime/node
module, which makes the file depend on the Node build and breaks browser runs;
either move the test case that uses getDefaultDataPath out of the shared
test/node-and-browser suite into the node-only suite (e.g., test/node) or remove
the static runtime/node import and instead gate a dynamic Node-only
import/require inside the test body (e.g., if (typeof process !== "undefined" &&
process.versions?.node) { const { getDefaultDataPath } = await
import(".../runtime/node/util.js"); ... }) so the module is only loaded when
running in Node. Ensure references to getDefaultDataPath are updated
accordingly.
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
1 participant
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.
Closes part of #79.
Summary
Implements the embedded half of
community.export(): ask any startedLocalCommunityto back itself up to a consistent sqlite snapshot. Records show up oncommunity.exportsand anexportschangeevent fires on every transition. Cancellation is viaAbortSignal.pkc.destroy()aborts in-flight exports.The RPC wire protocol + HTTP download endpoint + 24h orphan sweep are deferred to a follow-up PR (still tracked on #79) so this change stays reviewable.
Esteban's design feedback baked in
Per the comment on #79, the public
CommunityExportRecordexposes only:progress: number(0..1) — the only state indicatorsize,sha256,url— present onceprogress === 1error?: { code, message }— present on failure (cancellation surfaces aserror.code === 'ERR_EXPORT_CANCELLED')exportId,name?,publicKey,includePrivateKeyDropped from the earlier draft: explicit
stateenum,startedAt,pagesCopied,totalPages,completedAt. Consumers infer state fromprogressanderror. The spec doc notes these can be added back compatibly if a finer-grained state surface is ever needed.What's in this PR
src/rpc/EXPORT_COMMUNITY_SPEC.mdExportCommunityUserOptions,CommunityExportRecord(zod + TS) insrc/community/{schema,types}.tsbackupCommunityDb()insrc/runtime/node/util.ts— better-sqlite3 backup with AbortSignal-aware progress, atomic.partial→ final rename, sha256, optional private-key scrub of the signer KeyV recordsrc/runtime/node/community/local-community/export.ts— per-community serialization queue, KeyV persistence under a newSTORAGE_KEYS.EXPORTS, prune-on-load for files that vanished out of bandcommunity.export(),community.exports,exportschangeeventexport()rejects withERR_COMMUNITY_NOT_LOCAL;exportsreturns[])ERR_COMMUNITY_NOT_LOCAL,ERR_COMMUNITY_NOT_FOUND,ERR_PRIVATE_KEY_EXPORT_NOT_ALLOWED,ERR_EXPORT_PATH_NOT_SUPPORTED_OVER_RPC,ERR_EXPORT_CANCELLED,ERR_EXPORT_BACKUP_FAILED,ERR_DOWNLOAD_EXPORT_ID_NOT_FOUNDTest plan
npm run build— clean (node + browser + verify:browser-imports)npx tsc --project test/tsconfig.json --noEmit— cleannode test/run-test-config.js --pkc-config local-kubo-rpc test/node/community/export.test.tstest/node/community/create.community.test.tsstill greenlocal-kubo-rpcandremote-pkc-rpcconfigsThe new test cases cover: happy path (sha256 + readable sqlite),
includePrivateKey: falsescrubs signer,includePrivateKey: truepreserves it,exportPathoverride,exportschangeemissions, AbortSignal mid-flight cancellation, pre-aborted signal rejecting synchronously without a record, concurrent exports of the same community both completing,ERR_COMMUNITY_NOT_LOCALfor read-onlyRemoteCommunity, andpkc.destroy()teardown.Follow-up (still tracked on #79)
exportCommunity,exportsSubscribe,cancelExport) + schemasGET /exports/<exportId>download endpoint on the same port as the WS RPCtest/node-and-browser/rpc/exports.test.tsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation