Another attempt to fix conformance slowness#1023
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR changes fjall-backed pruning in Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workers/api-node/config.ts (1)
99-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAwait
blocks.close()andstates.close()before releasing the keyspace
FjallBlocksandFjallStatesboth drainpendingPruneinclose(). Skipping those calls can leave prune removals in flight when the shared-keyspace path tears down or deletes partitions, so the pruned entries may never be removed cleanly.🤖 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 `@packages/workers/api-node/config.ts` around lines 99 - 148, The openDatabase flow in config.ts closes the underlying Fjall keyspace without first closing the opened FjallBlocks and FjallStates instances, so their pendingPrune work can be dropped. Update openDatabase’s cleanup path, especially the returned close method and the error handling around the Promise.all opening block, to await blocks.close() and states.close() before calling fjall.close() or releasing the shared keyspace; use the existing blocks, states, and ownsFjall symbols to keep the teardown order correct.
🧹 Nitpick comments (1)
packages/jam/database-fjall/blocks.ts (1)
90-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate prune-queue pattern shared with
FjallStates.The same
pendingPrunechaining/close pattern is duplicated verbatim inpackages/jam/database-fjall/states.ts(Lines 89-100). Consider extracting a small shared helper (e.g. aPruneQueueutility withenqueue(fn)/close()) to avoid maintaining two copies of this logic.♻️ Sketch of a shared helper
// e.g. packages/jam/database-fjall/prune-queue.ts export class PruneQueue { private pending: Promise<unknown> = Promise.resolve(); enqueue(op: () => Promise<unknown>, onError: (e: unknown) => void): void { this.pending = this.pending.then(op).catch(onError); } async close(): Promise<void> { await this.pending; } }Then in
FjallBlocks:- private pendingPrune: Promise<unknown> = Promise.resolve(); + private readonly pruneQueue = new PruneQueue(); ... markUnused(hash: HeaderHash): void { - this.pendingPrune = this.pendingPrune - .then(() => - Promise.all([...]), - ) - .catch((e) => logger.warn`Failed to prune block ${hash}: ${e}`); + this.pruneQueue.enqueue( + () => Promise.all([...]), + (e) => logger.warn`Failed to prune block ${hash}: ${e}`, + ); } async close() { - await this.pendingPrune; + await this.pruneQueue.close(); }🤖 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 `@packages/jam/database-fjall/blocks.ts` around lines 90 - 104, The prune-queue logic in FjallBlocks.markUnused and close is duplicated in FjallStates, so extract the shared pendingPrune chaining and shutdown behavior into a small reusable helper such as PruneQueue with enqueue and close methods. Update FjallBlocks to use that helper instead of maintaining its own promise chain and error handling, and apply the same shared utility in FjallStates so both classes reuse one implementation.
🤖 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.
Outside diff comments:
In `@packages/workers/api-node/config.ts`:
- Around line 99-148: The openDatabase flow in config.ts closes the underlying
Fjall keyspace without first closing the opened FjallBlocks and FjallStates
instances, so their pendingPrune work can be dropped. Update openDatabase’s
cleanup path, especially the returned close method and the error handling around
the Promise.all opening block, to await blocks.close() and states.close() before
calling fjall.close() or releasing the shared keyspace; use the existing blocks,
states, and ownsFjall symbols to keep the teardown order correct.
---
Nitpick comments:
In `@packages/jam/database-fjall/blocks.ts`:
- Around line 90-104: The prune-queue logic in FjallBlocks.markUnused and close
is duplicated in FjallStates, so extract the shared pendingPrune chaining and
shutdown behavior into a small reusable helper such as PruneQueue with enqueue
and close methods. Update FjallBlocks to use that helper instead of maintaining
its own promise chain and error handling, and apply the same shared utility in
FjallStates so both classes reuse one implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4cb6dcf-c7f5-4b18-ad02-f14b2a59795c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
packages/jam/database-fjall/blocks.tspackages/jam/database-fjall/hybrid-states.tspackages/jam/database-fjall/package.jsonpackages/jam/database-fjall/root.test.tspackages/jam/database-fjall/root.tspackages/jam/database-fjall/states.tspackages/jam/node/main-fuzz.tspackages/jam/node/main-importer.tspackages/workers/api-node/config.test.tspackages/workers/api-node/config.ts
1 participant
No description provided.