ft. ENG-26 — Implement sequence counter helpers for monotonic gap-free numbering#84
Merged
Connorbelez merged 3 commits intomainfrom Mar 16, 2026
Merged
ft. ENG-26 — Implement sequence counter helpers for monotonic gap-free numbering#84Connorbelez merged 3 commits intomainfrom
Connorbelez merged 3 commits intomainfrom
Conversation
…e numbering Replace query-last-entry sequence generation with a singleton counter document pattern. Every journal write now touches a dedicated counter doc, creating an explicit OCC serialization point per the spec. - Add ledger_sequence_counters table to schema - Create convex/ledger/sequenceCounter.ts with initializeSequenceCounter (idempotent ledgerMutation) and getNextSequenceNumber (ConvexError on uninitialized) - Remove old nextSequenceNumber from internal.ts - Update mutations.ts and demo/ledger.ts to use new helper - Add 5 unit tests (init, idempotency, monotonic, gap-free, error) - Update all 45 existing ledger tests to initialize counter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe changes refactor ledger sequence number generation from a query-based approach to a dedicated singleton counter table. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Reviewer's GuideReplaces query-based journal sequencing with a singleton ledger_sequence_counters table and helper module that provides an explicitly initialized, mutation-scoped, monotonic, gap-free sequence counter used by all ledger journal writes, with comprehensive tests and test harness updates to require counter initialization. Sequence diagram for initializing the ledger sequence countersequenceDiagram
actor Admin
participant InitializeSequenceCounterMutation
participant Db as ledger_sequence_counters
Admin->>InitializeSequenceCounterMutation: call initializeSequenceCounter(ctx)
InitializeSequenceCounterMutation->>Db: query by_name name=ledger_sequence
Db-->>InitializeSequenceCounterMutation: existing or null
alt counter_exists
InitializeSequenceCounterMutation-->>Admin: return existing._id
else counter_missing
InitializeSequenceCounterMutation->>Db: insert name=ledger_sequence, value=0
Db-->>InitializeSequenceCounterMutation: new_id
InitializeSequenceCounterMutation-->>Admin: return new_id
end
Sequence diagram for posting a ledger entry with monotonic gap-free numberingsequenceDiagram
actor Client
participant LedgerMutation as postEntryInternal
participant SequenceHelper as getNextSequenceNumber
participant DbCounter as ledger_sequence_counters
participant DbJournal as ledger_journal_entries
Client->>LedgerMutation: call postEntryInternal(ctx, args)
LedgerMutation->>SequenceHelper: getNextSequenceNumber(ctx)
SequenceHelper->>DbCounter: query by_name name=ledger_sequence
alt counter_not_initialized
DbCounter-->>SequenceHelper: null
SequenceHelper-->>LedgerMutation: throw ConvexError
LedgerMutation-->>Client: error SEQUENCE_COUNTER_NOT_INITIALIZED
else counter_initialized
DbCounter-->>SequenceHelper: counter{name, value}
SequenceHelper->>SequenceHelper: nextValue = value + 1
SequenceHelper->>DbCounter: patch counter.value = nextValue
DbCounter-->>SequenceHelper: ok
SequenceHelper-->>LedgerMutation: nextValue
LedgerMutation->>DbJournal: insert journal_entry(sequenceNumber=nextValue, ...)
DbJournal-->>LedgerMutation: entry_id
LedgerMutation-->>Client: success with entry_id
end
ER diagram for ledger_sequence_counters and journal entrieserDiagram
ledger_sequence_counters {
string name
bigint value
}
ledger_journal_entries {
bigint sequenceNumber
string entryType
string description
float amount
string debitAccountId
string creditAccountId
int64 createdAt
}
ledger_sequence_counters ||--o{ ledger_journal_entries : assigns_sequence_numbers
Class diagram for ledger sequence counter helpersclassDiagram
class LedgerSequenceCounterTable {
string name
bigint value
}
class InitializeSequenceCounterMutation {
+initializeSequenceCounter(ctx)
}
class GetNextSequenceNumberHelper {
+getNextSequenceNumber(ctx) bigint
}
class LedgerMutationsModule {
+postEntryInternal(ctx, args)
}
class DemoLedgerModule {
+postSeedEntry(ctx, args)
}
LedgerSequenceCounterTable <.. InitializeSequenceCounterMutation : queries_and_inserts
LedgerSequenceCounterTable <.. GetNextSequenceNumberHelper : queries_and_patches
GetNextSequenceNumberHelper <.. LedgerMutationsModule : uses
GetNextSequenceNumberHelper <.. DemoLedgerModule : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
initializeSequenceCountermutation is exposed as.public(), which means any caller with ledger access can (re)invoke it; consider restricting this to an admin-only or migration-only path so that the singleton counter cannot be tampered with from normal client flows. - Right now the sequence counter must be explicitly initialized and you wired that into the tests, but the demo/seed and any production bootstrap path still rely on
getNextSequenceNumberand will now throw until initialization happens; consider adding a clear initialization hook (e.g. in seed/bootstrap or a migration) so non-test usage isn’t broken by the new ConvexError. - You added an
initCounterhelper local toledger.test.tsand then call it in many tests; consider moving this into a shared test utility or wrapping it intocreateTestHarness/asLedgerUserto reduce repetition and ensure future tests can’t forget to initialize the counter.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `initializeSequenceCounter` mutation is exposed as `.public()`, which means any caller with ledger access can (re)invoke it; consider restricting this to an admin-only or migration-only path so that the singleton counter cannot be tampered with from normal client flows.
- Right now the sequence counter must be explicitly initialized and you wired that into the tests, but the demo/seed and any production bootstrap path still rely on `getNextSequenceNumber` and will now throw until initialization happens; consider adding a clear initialization hook (e.g. in seed/bootstrap or a migration) so non-test usage isn’t broken by the new ConvexError.
- You added an `initCounter` helper local to `ledger.test.ts` and then call it in many tests; consider moving this into a shared test utility or wrapping it into `createTestHarness`/`asLedgerUser` to reduce repetition and ensure future tests can’t forget to initialize the counter.
## Individual Comments
### Comment 1
<location path="convex/ledger/sequenceCounter.ts" line_range="11-20" />
<code_context>
+ * Bootstrap mutation: creates the singleton counter document with value 0.
+ * Idempotent — safe to call multiple times.
+ */
+export const initializeSequenceCounter = ledgerMutation
+ .handler(async (ctx) => {
+ const existing = await ctx.db
+ .query("ledger_sequence_counters")
+ .withIndex("by_name", (q) => q.eq("name", COUNTER_NAME))
+ .unique();
+
+ if (existing) {
+ return existing._id;
+ }
+
+ return ctx.db.insert("ledger_sequence_counters", {
+ name: COUNTER_NAME,
+ value: 0n,
</code_context>
<issue_to_address>
**issue (bug_risk):** The initialization mutation can race and create multiple counter documents, breaking `.unique()` assumptions.
Because this does a read followed by an insert, concurrent calls can both see `existing === null` and each insert a `ledger_sequence_counters` document with the same `name`. That would leave multiple matching documents and cause subsequent `.unique()` calls here and in `getNextSequenceNumber` to throw. Consider a race-safe pattern: e.g., attempt an insert and handle duplicate-key-like errors by re-reading, or enforce stronger uniqueness in the schema/logic and retry after a failed insert.
</issue_to_address>
### Comment 2
<location path="convex/ledger/__tests__/ledger.test.ts" line_range="927" />
<code_context>
describe("Idempotency & Sequencing", () => {
it("T-065: same idempotencyKey returns existing entry, no double-post", async () => {
const t = createTestHarness();
+ await initCounter(t);
</code_context>
<issue_to_address>
**issue (testing):** Add an assertion that reusing the same idempotencyKey does not advance the sequence counter, to prove gap-free numbering under idempotent replays.
Since this test already covers `idempotencyKey` behavior, please also assert that reusing the same key does not change the journal entry’s `sequenceNumber`. For example, capture the `sequenceNumber` from the first call, invoke the same operation with the same `idempotencyKey`, and assert that the returned entry has the identical `sequenceNumber`. This will verify that idempotent retries do not introduce gaps in the new counter-based sequencing.
</issue_to_address>
### Comment 3
<location path="convex/ledger/__tests__/sequenceCounter.test.ts" line_range="56-76" />
<code_context>
+ expect(doc!.value).toBe(0n);
+ });
+
+ it("initializeSequenceCounter is idempotent", async () => {
+ const t = createTestHarness();
+ const auth = asLedgerUser(t);
+
+ const id1 = await auth.mutation(
+ api.ledger.sequenceCounter.initializeSequenceCounter,
+ {},
+ );
+ const id2 = await auth.mutation(
+ api.ledger.sequenceCounter.initializeSequenceCounter,
+ {},
+ );
+
+ expect(id1).toBe(id2);
+
+ // Verify only one document exists
+ const docs = await t.run(async (ctx) => {
+ return ctx.db.query("ledger_sequence_counters").collect();
+ });
+ expect(docs).toHaveLength(1);
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the idempotency test to verify that `initializeSequenceCounter` does not reset an already-incremented counter value.
After the first initialization, trigger a mutation that calls `getNextSequenceNumber` to increment the counter, then invoke `initializeSequenceCounter` again and assert that the stored `value` remains the incremented value. This will help catch regressions where initialization might overwrite an existing sequence value.
```suggestion
it("initializeSequenceCounter is idempotent", async () => {
const t = createTestHarness();
const auth = asLedgerUser(t);
const id1 = await auth.mutation(
api.ledger.sequenceCounter.initializeSequenceCounter,
{},
);
// Increment the sequence counter after initialization
await auth.mutation(
api.ledger.sequenceCounter.getNextSequenceNumber,
{},
);
const id2 = await auth.mutation(
api.ledger.sequenceCounter.initializeSequenceCounter,
{},
);
expect(id1).toBe(id2);
// Verify only one document exists and that the value was not reset
const docs = await t.run(async (ctx) => {
return ctx.db.query("ledger_sequence_counters").collect();
});
expect(docs).toHaveLength(1);
expect(docs[0]!.value).toBe(1n);
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR replaces the previous query-last-entry sequence generation strategy with a singleton counter document in a new
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant ledgerMutation
participant postEntryInternal
participant sequenceCounter
participant DB
Note over Client,DB: Prerequisite: initializeSequenceCounter must be called first
Client->>ledgerMutation: initializeSequenceCounter()
ledgerMutation->>DB: query ledger_sequence_counters (by_name)
alt counter exists
DB-->>ledgerMutation: existing doc
ledgerMutation-->>Client: existing._id (idempotent)
else not initialized
ledgerMutation->>DB: insert { name: "ledger_sequence", value: 0n }
DB-->>ledgerMutation: new _id
ledgerMutation-->>Client: new _id
end
Note over Client,DB: Normal ledger write flow
Client->>ledgerMutation: mintMortgage / issueShares / etc.
ledgerMutation->>postEntryInternal: postEntryInternal(ctx, args)
postEntryInternal->>DB: idempotency check (by_idempotency)
postEntryInternal->>DB: load debit + credit accounts
postEntryInternal->>postEntryInternal: validateEntryType()
postEntryInternal->>sequenceCounter: getNextSequenceNumber(ctx)
sequenceCounter->>DB: query ledger_sequence_counters (by_name)
alt counter missing
sequenceCounter-->>postEntryInternal: throw ConvexError(NOT_INITIALIZED)
postEntryInternal-->>Client: error
else counter found
sequenceCounter->>DB: patch counter { value: value + 1n }
sequenceCounter-->>postEntryInternal: nextValue (bigint)
postEntryInternal->>DB: insert ledger_journal_entries { sequenceNumber: nextValue }
postEntryInternal->>DB: patch debitAccount cumulativeDebits
postEntryInternal->>DB: patch creditAccount cumulativeCredits
postEntryInternal-->>ledgerMutation: journalEntry doc
ledgerMutation-->>Client: result
end
|
There was a problem hiding this comment.
Pull request overview
Implements a dedicated singleton sequence counter document for ledger journal entry numbering, replacing the prior “query latest journal entry” approach to ensure monotonic, gap-free sequencing via an explicit OCC serialization point.
Changes:
- Adds
ledger_sequence_counterstable to the Convex schema and a newconvex/ledger/sequenceCounter.tshelper (init + next sequence). - Updates ledger journal write paths to use
getNextSequenceNumberand removes the oldnextSequenceNumberhelper. - Adds new unit tests for the counter and updates existing ledger tests to initialize the counter.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| convex/seed/seedHelpers.ts | Formatting-only change in an indexed query. |
| convex/schema.ts | Adds ledger_sequence_counters table + index. |
| convex/ledger/sequenceCounter.ts | New initialization mutation and next-sequence helper. |
| convex/ledger/mutations.ts | Switches journal entry sequencing to the new helper. |
| convex/ledger/internal.ts | Removes the old nextSequenceNumber implementation. |
| convex/ledger/tests/sequenceCounter.test.ts | Adds focused tests for init/idempotency/monotonic/gap-free/error cases. |
| convex/ledger/tests/ledger.test.ts | Introduces initCounter helper and calls it in tests before ledger mutations. |
| convex/demo/ledger.ts | Demo seed journal writes now use the new counter-based sequencing. |
| convex/_generated/api.d.ts | Updates generated API typings to include ledger/sequenceCounter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@greptile please review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
convex/ledger/__tests__/ledger.test.ts (1)
43-50: Recommended: centralize initialized test harness creation.
initCounteris good, but repeatingawait initCounter(t)across many tests is easy to miss in future additions. Prefer a single helper that returns an already-initialized harness.♻️ Suggested refactor
+async function createInitializedHarness() { + const t = createTestHarness(); + await initCounter(t); + return { t, auth: asLedgerUser(t) }; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/ledger/__tests__/ledger.test.ts` around lines 43 - 50, Create a single helper that returns a test harness with the ledger sequence counter already initialized instead of calling initCounter(t) in every test; for example add a function (e.g., createInitializedHarness) that calls createTestHarness(), then runs asLedgerUser(harness).mutation(api.ledger.sequenceCounter.initializeSequenceCounter, {}) and returns the harness so tests can use it directly—update tests to import/use createInitializedHarness (or modify createTestHarness to perform the init) and remove repeated await initCounter(t) calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/ledger/sequenceCounter.ts`:
- Around line 34-45: The getNextSequenceNumber function currently throws
SEQUENCE_COUNTER_NOT_INITIALIZED if the ledger_sequence_counters row is missing;
update it to automatically initialize the counter on first access or provide a
bootstrap migration/HTTP endpoint that calls initializeSequenceCounter.
Specifically, inside getNextSequenceNumber (and any callers in
convex/ledger/mutations.ts and convex/demo/ledger.ts) detect a missing counter
(ledger_sequence_counters with name COUNTER_NAME) and either invoke
initializeSequenceCounter to insert the initial row before proceeding or
document/implement a startup migration that ensures initializeSequenceCounter
runs during deployment; keep the ConvexError only for truly unrecoverable cases
and ensure COUNTER_NAME and ledger_sequence_counters are used consistently.
---
Nitpick comments:
In `@convex/ledger/__tests__/ledger.test.ts`:
- Around line 43-50: Create a single helper that returns a test harness with the
ledger sequence counter already initialized instead of calling initCounter(t) in
every test; for example add a function (e.g., createInitializedHarness) that
calls createTestHarness(), then runs
asLedgerUser(harness).mutation(api.ledger.sequenceCounter.initializeSequenceCounter,
{}) and returns the harness so tests can use it directly—update tests to
import/use createInitializedHarness (or modify createTestHarness to perform the
init) and remove repeated await initCounter(t) calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a258703-9d89-488f-89b4-a79477cb932c
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (8)
convex/demo/ledger.tsconvex/ledger/__tests__/ledger.test.tsconvex/ledger/__tests__/sequenceCounter.test.tsconvex/ledger/internal.tsconvex/ledger/mutations.tsconvex/ledger/sequenceCounter.tsconvex/schema.tsconvex/seed/seedHelpers.ts
💤 Files with no reviewable changes (1)
- convex/ledger/internal.ts
…rap migration Addresses PR review feedback: - Replace .unique() with .first() in both initializeSequenceCounter and getNextSequenceNumber to be resilient against duplicate documents - Add bootstrapSequenceCounter admin mutation in migrations.ts as a production bootstrap path for fresh deployments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extend idempotency test to verify init doesn't reset incremented counter - Add assertion that idempotent replay doesn't advance sequence number - Add counter initialization to demo seedData for fresh DB support Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 16, 2026
Merged
Merged
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
…e numbering (#84) Replace query-last-entry sequence generation with a singleton counter document pattern. Every journal write now touches a dedicated counter doc, creating an explicit OCC serialization point per the spec. - Add ledger_sequence_counters table to schema - Create convex/ledger/sequenceCounter.ts with initializeSequenceCounter (idempotent ledgerMutation) and getNextSequenceNumber (ConvexError on uninitialized) - Remove old nextSequenceNumber from internal.ts - Update mutations.ts and demo/ledger.ts to use new helper - Add 5 unit tests (init, idempotency, monotonic, gap-free, error) - Update all 45 existing ledger tests to initialize counter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> ## Summary by Sourcery Introduce a dedicated ledger sequence counter backed by a singleton table and wire all ledger writes to use it for monotonic, gap-free journal numbering. New Features: - Add a ledger_sequence_counters table and sequenceCounter module to manage a singleton ledger sequence counter with explicit initialization and next-number helper. Bug Fixes: - Prevent race conditions and gaps in ledger journal sequence numbers by replacing query-last-entry sequencing with a centralized counter document. Tests: - Add focused unit tests for sequence counter initialization, idempotency, monotonicity, gap-freedom, and error-on-uninitialized, and update existing ledger tests to initialize the counter before mutations. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added comprehensive test suite for ledger sequence counter initialization and monotonic number generation, ensuring sequence number consistency across operations. * **Chores** * Refactored internal sequence number generation mechanism for improved maintainability and reliability. * Added new sequence counter table to data schema. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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.
Replace query-last-entry sequence generation with a singleton counter
document pattern. Every journal write now touches a dedicated counter
doc, creating an explicit OCC serialization point per the spec.
(idempotent ledgerMutation) and getNextSequenceNumber (ConvexError on
uninitialized)
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by Sourcery
Introduce a dedicated ledger sequence counter backed by a singleton table and wire all ledger writes to use it for monotonic, gap-free journal numbering.
New Features:
Bug Fixes:
Tests:
Summary by CodeRabbit
Tests
Chores