feat(branch): kv::tenancy — managed KV residency, topology, RESTRICT/…#17
feat(branch): kv::tenancy — managed KV residency, topology, RESTRICT/…#17lloyal-research merged 4 commits intomainfrom
Conversation
…CASCADE Seq_id allocation moves from caller to BranchStore. kv::tenancy tracks leases internally — acquired on create()/fork(), evicted on prune(), rebuilt on retainOnly(). No manual seq_id tracking at the JS layer. N-API (SessionContext.cpp): - branchCreate/branchFork no longer take seqId param - Remove _branchDestroy, _branchGetSeqId (seq_id is internal) - Add _branchPruneSubtree, _branchParent, _branchChildren, _branchIsLeaf, _branchIsActive, _storeRetainOnly, _storeAvailable - init_tenancy() on context creation, drain() before dispose() JS (Branch.js, BranchStore.js): - Branch.create() / fork() drop seqId parameter - Add pruneSubtree(), parent, children, isLeaf, isActive - BranchStore gains retainOnly(winner), available getter - Remove destroy(), seqId getter
There was a problem hiding this comment.
Pull request overview
This PR introduces managed KV residency and topology tracking for branches, moving seq_id allocation from JavaScript to C++ and automating lease lifecycle management. The key architectural shift is that kv::tenancy now internally tracks leases — acquired on create()/fork(), evicted on prune(), rebuilt on retainOnly() — eliminating manual seq_id tracking at the JS layer.
Changes:
- N-API methods now omit
seqIdparameters frombranchCreate/branchForkand remove_branchGetSeqId/_branchDestroy - Added topology introspection methods (
_branchParent,_branchChildren,_branchIsLeaf,_branchIsActive) and CASCADE operations (_branchPruneSubtree,_storeRetainOnly) - JavaScript API updated to remove
seqIdparameter fromBranch.create()/fork()and replacedestroy()withpruneSubtree()
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration.js | Updated all Branch.create() and fork() calls to omit seqId parameter; changed prune order and added pruneSubtree() usage; increased nSeqMax limits |
| src/SessionContext.hpp | Removed _branchGetSeqId and _branchDestroy method declarations; added new topology methods (_branchPruneSubtree, _branchParent, _branchChildren, _branchIsLeaf, _branchIsActive) and store methods (_storeRetainOnly, _storeAvailable) |
| src/SessionContext.cpp | Implemented API signature changes removing seqId parameters; changed &_branchStore to _branchStore reference passing; added init_tenancy() initialization and drain() cleanup; implemented new topology and store methods |
| liblloyal | Updated subproject commit reference to version supporting new tenancy features |
| lib/index.d.ts | Updated TypeScript definitions to remove seqId parameters and add new topology/store methods; updated documentation comments |
| lib/BranchStore.js | Added retainOnly() method and available getter |
| lib/Branch.js | Removed seqId parameter from create()/fork(); replaced destroy() with pruneSubtree(); added parent, children, isLeaf, isActive getters; updated documentation |
| README.md | Rewrote documentation to emphasize continuous tree batching, KV tenancy, and topology; added badges and comprehensive examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nCtx: CTX_SIZE, | ||
| nThreads: 4 | ||
| nThreads: 4, | ||
| nSeqMax: 8 |
There was a problem hiding this comment.
The nSeqMax value increased from 4 to 8 in testBranchSteer but the test only uses 2 sequences (parent and child). This seems like an arbitrary change that doesn't align with the test's actual requirements. Consider using a value that matches the test's needs or document why 8 is necessary.
| nSeqMax: 8 | |
| nSeqMax: 2 |
There was a problem hiding this comment.
Technically correct — the test only uses 1 branch. However, nSeqMax: 8 is consistent with testBranchStore and provides headroom if branches are added later. Not worth tightening for a test helper.
| nBatch: 512, | ||
| nThreads: 4, | ||
| nSeqMax: 4 | ||
| nSeqMax: 8 |
There was a problem hiding this comment.
The nSeqMax value increased from 4 to 8 in testBranchStore. Given that Test A uses 3 branches, Tests B-F use 2 branches each, this increase to 8 appears arbitrary and doesn't reflect actual usage. Consider setting it to match the maximum concurrent branches used (3) or document the rationale.
| nSeqMax: 8 | |
| nSeqMax: 3 |
There was a problem hiding this comment.
Test A uses root + 2 forks = 3 concurrent leases, so nSeqMax: 3 is the bare minimum with zero headroom. Adding one fork to any test would break the suite. 8 is deliberate breathing room for a function that exercises 6 different branching scenarios.
| `rehydrate: perplexity valid after prefill→commit (b1=${b1.perplexity.toFixed(2)}, b2=${b2.perplexity.toFixed(2)})`); | ||
|
|
||
| b1.prune(); b2.prune(); | ||
| b2.prune(); b1.prune(); |
There was a problem hiding this comment.
The prune order was changed from b1.prune(); b2.prune() to b2.prune(); b1.prune() in multiple test cases. While this may be intentional to test child-before-parent ordering with the new tenancy system, the inconsistency across tests (lines 1117, 1187, 1232, 1292) suggests this might be incidental rather than deliberate. Consider documenting why prune order matters or maintaining consistent ordering if it doesn't.
| b2.prune(); b1.prune(); | |
| b1.prune(); b2.prune(); |
There was a problem hiding this comment.
This reversal is required, not incidental. b1 is the parent (Branch.create), b2 is the child (b1.fork()). With RESTRICT semantics introduced by kv::tenancy, prune() throws if the branch has children — you must prune child before parent. The old order only worked because pre-tenancy prune() had no topology enforcement. Reverting this suggestion would break all affected tests at runtime.
…CASCADE
Seq_id allocation moves from caller to BranchStore. kv::tenancy tracks leases internally — acquired on create()/fork(), evicted on prune(), rebuilt on retainOnly(). No manual seq_id tracking at the JS layer.
N-API (SessionContext.cpp):
JS (Branch.js, BranchStore.js):