From feb68240c6fe0e488cf656a2e9e3389deeb61ee7 Mon Sep 17 00:00:00 2001 From: MikeyZhang75 Date: Fri, 21 Aug 2026 23:26:39 +0800 Subject: [PATCH 1/5] test(orm): count documents scanned behind .filter() countDocumentReads counted rows returned, never rows Convex read and a .filter() rejected, so read bounds on filter paths could not fail. Adds scanned alongside documents: reads the serialized plan off QueryImpl.state.query before the terminal consumes it, replays the source with no operators, and charges the walk. Early stop modelled, unrecognized plan shapes throw instead of under-reporting. Moves every read bound to scanned, re-seeds M4 at N=300. Fixes #384 --- .../example-project-existence-reads.test.ts | 4 +- convex/orm/example-tag-merge-reads.test.ts | 4 +- convex/orm/ordering.test.ts | 4 +- convex/orm/pipeline.test.ts | 16 +- convex/orm/read-counting.test.ts | 325 +++++++++++++ convex/orm/relation-loading.test.ts | 16 +- convex/orm/rls.test.ts | 8 +- convex/orm/where-filtering.test.ts | 22 +- convex/setup.testing.ts | 286 ++++++++++- ...an-aware-read-counting-for-filter-paths.md | 449 ++++++++++++++++++ 10 files changed, 1077 insertions(+), 57 deletions(-) create mode 100644 convex/orm/read-counting.test.ts create mode 100644 docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md diff --git a/convex/orm/example-project-existence-reads.test.ts b/convex/orm/example-project-existence-reads.test.ts index 63c26c200..bab4131f7 100644 --- a/convex/orm/example-project-existence-reads.test.ts +++ b/convex/orm/example-project-existence-reads.test.ts @@ -91,7 +91,7 @@ test('project existence check does not scan projects owned by other users', asyn await expect(hasAnyProject(ctx, viewer.id)).resolves.toBe(false); // Both legs hit an index range that is empty for this user. The // unindexed read model reported OTHER_USER_PROJECTS reads here. - expect(reads.documents).toBeLessThanOrEqual(2); + expect(reads.scanned).toBeLessThanOrEqual(2); }); }); }); @@ -150,7 +150,7 @@ test('project existence check reads only the viewer own and member rows', async await expect(hasAnyProject(ctx, viewer.id)).resolves.toBe(true); // One membership and one related project row. Nothing proportional to // OTHER_USER_PROJECTS. - expect(reads.documents).toBeLessThanOrEqual(3); + expect(reads.scanned).toBeLessThanOrEqual(3); }); }); }); diff --git a/convex/orm/example-tag-merge-reads.test.ts b/convex/orm/example-tag-merge-reads.test.ts index 444928297..c30f13497 100644 --- a/convex/orm/example-tag-merge-reads.test.ts +++ b/convex/orm/example-tag-merge-reads.test.ts @@ -115,7 +115,7 @@ test('merge cost does not track the target tag size', async () => { sourceTagId, targetTagId, }); - const mergeReads = reads.documents; + const mergeReads = reads.scanned; const [sourceTag, targetJoins] = await Promise.all([ ctx.orm.query.tags.findFirst({ where: { id: sourceTagId } }), @@ -181,7 +181,7 @@ test('merge probe finds a target row the source todo already carries', async () expect(targetJoins).toHaveLength(1); // The probe is index-backed: the merge must not scan the target's other // rows or insert a duplicate for the shared todo. - expect(reads.documents).toBeLessThanOrEqual(20); + expect(reads.scanned).toBeLessThanOrEqual(20); }); }); }); diff --git a/convex/orm/ordering.test.ts b/convex/orm/ordering.test.ts index b4f9d6742..629c979f3 100644 --- a/convex/orm/ordering.test.ts +++ b/convex/orm/ordering.test.ts @@ -654,7 +654,7 @@ describe('M5: OrderBy - Index Selection', () => { expect(users.map((user) => user.name)).toEqual(['u59', 'u58']); // One bounded read per probe, not the whole matching set. - expect(reads.documents).toBeLessThanOrEqual(8); + expect(reads.scanned).toBeLessThanOrEqual(8); }); test('orderBy an index suffix field reads only the requested window', async ({ @@ -678,7 +678,7 @@ describe('M5: OrderBy - Index Selection', () => { }); expect(posts.map((post) => post.numLikes)).toEqual([59, 58]); - expect(reads.documents).toBeLessThanOrEqual(4); + expect(reads.scanned).toBeLessThanOrEqual(4); }); test('orderBy createdAt under an unpinned index suffix sorts by createdAt', async ({ diff --git a/convex/orm/pipeline.test.ts b/convex/orm/pipeline.test.ts index e4adfe061..f18d68be4 100644 --- a/convex/orm/pipeline.test.ts +++ b/convex/orm/pipeline.test.ts @@ -583,7 +583,7 @@ test('flatMap limit reads each child once and stays under maxScan', async () => .paginate({ cursor: null, limit: 5, maxScan: 6 }); // 1 parent + at most 6 child reads for maxScan: 6. - expect(reads.documents).toBeLessThanOrEqual(7); + expect(reads.scanned).toBeLessThanOrEqual(7); // And that walk has to be visible to maxScan, not happen behind it. expect(result.pageStatus).toBe('SplitRequired'); }); @@ -619,7 +619,7 @@ test('flatMap limit stops on the limit-th child, not one past it', async () => { expect(rows.map((row) => row.text)).toEqual(['p0', 'p1']); // 1 parent + exactly 2 children. A third child read would be a document // nothing can emit and maxScan never sees, since only replayed rows count. - expect(reads.documents).toBe(3); + expect(reads.scanned).toBe(3); }); }); @@ -730,7 +730,7 @@ test('select() pipeline reads an id-only where by key, not by scan', async () => .limit(10); expect(mapped).toEqual([{ onlyTitle: 'title-39' }]); - expect(reads.documents).toBe(1); + expect(reads.scanned).toBe(1); }); }); @@ -766,7 +766,7 @@ test('select() pipeline reads an id `in` where by key, in list order', async () { onlyTitle: 'title-30' }, { onlyTitle: 'title-5' }, ]); - expect(reads.documents).toBe(2); + expect(reads.scanned).toBe(2); }); }); @@ -839,7 +839,7 @@ test('select() pages an id list without re-reading the whole list', async () => .where({ id: { in: ids } }) .map((row) => ({ onlyTitle: row.title })) .paginate({ cursor, limit: 10 }); - perPageReads.push(reads.documents); + perPageReads.push(reads.scanned); walked.push(...result.page.map((row) => row.onlyTitle as string)); cursor = result.continueCursor; if (result.isDone) { @@ -884,7 +884,7 @@ test('select() honors maxScan on id-list pagination', async () => { expect(result.page).toHaveLength(1); expect(result.isDone).toBe(false); - expect(reads.documents).toBe(1); + expect(reads.scanned).toBe(1); }); }); @@ -921,7 +921,7 @@ test('select() honors maxScan on an id list ordered by an index', async () => { .orderBy({ title: 'asc' }) .map((row) => ({ onlyTitle: row.title })) .paginate({ cursor, limit: 2, maxScan: 2 }); - perPageReads.push(reads.documents); + perPageReads.push(reads.scanned); walked.push(...result.page.map((row) => row.onlyTitle as string)); cursor = result.continueCursor; if (result.isDone) { @@ -1033,7 +1033,7 @@ test('select() flatMap runs off an id-only where without scanning', async () => expect(rows.map((row) => row.text)).toEqual(['Bo0', 'Bo1', 'Bo2']); // 1 parent read by key + its 3 children. Ada is never touched. - expect(reads.documents).toBe(4); + expect(reads.scanned).toBe(4); }); }); diff --git a/convex/orm/read-counting.test.ts b/convex/orm/read-counting.test.ts new file mode 100644 index 000000000..90701201f --- /dev/null +++ b/convex/orm/read-counting.test.ts @@ -0,0 +1,325 @@ +/** + * Read counting harness - `countDocumentReads` + * + * The counter exists so read bounds can fail when an index-backed plan degrades + * into a scan. It only does that if it can see documents Convex read and threw + * away: a `.filter()` rejects rows after the read is paid for, and those rows + * never reach `collect()`, `take()`, or the async iterator. `documents` counts + * what came back, `scanned` counts what it cost. + */ + +import { test as baseTest, describe, expect } from 'vitest'; +import schema from '../schema'; +import { + convexTest, + countDocumentReads, + runCtx, + type TestCtx, +} from '../setup.testing'; + +const test = baseTest.extend<{ ctx: TestCtx }>({ + ctx: async ({}, use) => { + const t = convexTest(schema); + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + await use(ctx); + }); + }, +}); + +const TABLE_SIZE = 300; + +/** `TABLE_SIZE` rows in one `by_status` bucket, named `u0`..`u299` in order. */ +const seedBucket = async (ctx: TestCtx) => { + for (let i = 0; i < TABLE_SIZE; i += 1) { + await ctx.db.insert('users', { + name: `u${i}`, + email: `u${i}@example.com`, + status: 'bucket', + }); + } +}; + +describe('countDocumentReads: rows rejected by .filter()', () => { + test('collect under a filter pays for the whole range', async ({ ctx }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const rows = await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u5')) + .collect(); + + expect(rows).toHaveLength(1); + expect(reads.documents).toBe(1); + // The other 299 rows were read and discarded by the predicate. + expect(reads.scanned).toBe(TABLE_SIZE); + }); + + test('take stops at the last match instead of draining the range', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const rows = await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u5')) + .take(1); + + expect(rows).toHaveLength(1); + expect(reads.documents).toBe(1); + // `u5` is the sixth row in index order, and `take(1)` is satisfied there. + expect(reads.scanned).toBe(6); + }); + + test('take that never fills its budget drains the range', async ({ ctx }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const rows = await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u5')) + .take(2); + + expect(rows).toHaveLength(1); + expect(reads.documents).toBe(1); + // Only one row matches, so the second slot is chased to the end. + expect(reads.scanned).toBe(TABLE_SIZE); + }); + + test('first stops at its match', async ({ ctx }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const row = await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u5')) + .first(); + + expect(row?.name).toBe('u5'); + expect(reads.documents).toBe(1); + expect(reads.scanned).toBe(6); + }); + + test('unique proves the match is alone by reading the rest', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const row = await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u5')) + .unique(); + + expect(row?.name).toBe('u5'); + expect(reads.documents).toBe(1); + // A second match would have made this ambiguous, so the range is drained. + expect(reads.scanned).toBe(TABLE_SIZE); + }); + + test('a filtered async iterator charges for the rows it skipped', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const names: string[] = []; + for await (const row of ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u5'))) { + names.push(row.name); + break; + } + + expect(names).toEqual(['u5']); + expect(reads.documents).toBe(1); + // Abandoning the loop stops the read where the consumer stopped. + expect(reads.scanned).toBe(6); + }); + + test('a filtered iterator run to done stops on a satisfied limit', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const names: string[] = []; + // `.limit()` is what `take()` pushes into the plan, so an iterator that + // reaches `done` because the limit was met must not be charged for the + // rest of the range. + const query = ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => + q.or(q.eq(q.field('name'), 'u2'), q.eq(q.field('name'), 'u7')) + ) as any; + for await (const row of query.limit(1)) { + names.push(row.name); + } + + expect(names).toEqual(['u2']); + expect(reads.documents).toBe(1); + expect(reads.scanned).toBe(3); + }); + + test('a filtered page charges for the walk, not the page', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const page = await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => + q.or(q.eq(q.field('name'), 'u10'), q.eq(q.field('name'), 'u20')) + ) + .paginate({ cursor: null, numItems: 2 }); + + expect(page.page.map((row) => row.name)).toEqual(['u10', 'u20']); + expect(reads.documents).toBe(2); + // Twenty one rows had to be read to fill a two row page. + expect(reads.scanned).toBe(21); + }); +}); + +describe('countDocumentReads: paths with nothing hidden', () => { + test('an unfiltered range costs exactly what it returns', async ({ ctx }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const rows = await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .take(5); + + expect(rows).toHaveLength(5); + expect(reads.documents).toBe(5); + expect(reads.scanned).toBe(5); + }); + + test('db.get costs one document', async ({ ctx }) => { + const id = await ctx.db.insert('users', { + name: 'solo', + email: 'solo@example.com', + }); + + const reads = countDocumentReads(ctx); + await ctx.db.get(id); + + expect(reads.documents).toBe(1); + expect(reads.scanned).toBe(1); + }); + + test('a JavaScript stream filter is already fully visible', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const rows = await ctx.orm.query.users.withIndex('by_name').findMany({ + where: { name: { endsWith: '5' } }, + }); + + expect(rows.length).toBeGreaterThan(0); + // `endsWith` cannot ride a Convex filter, so it runs in JavaScript over a + // stream: every candidate row is pulled through the iterator and was + // already visible before scan accounting existed. + expect(reads.scanned).toBe(TABLE_SIZE); + expect(reads.documents).toBe(TABLE_SIZE); + }); +}); + +describe('countDocumentReads: plan degradation is now assertable', () => { + test('an OR across two indexed fields is billed for the table it scans', async ({ + ctx, + }) => { + for (let i = 0; i < TABLE_SIZE; i += 1) { + await ctx.db.insert('users', { + name: `u${i}`, + email: `u${i}@example.com`, + status: 'filler', + age: 1, + }); + } + await ctx.db.insert('users', { + name: 'by-status', + email: 'by-status@example.com', + status: 'zmatch', + age: 1, + }); + await ctx.db.insert('users', { + name: 'by-age', + email: 'by-age@example.com', + status: 'filler', + age: 99, + }); + + const reads = countDocumentReads(ctx); + const rows = await ctx.orm.query.users.findMany({ + allowFullScan: true, + where: { OR: [{ status: 'zmatch' }, { age: 99 }] }, + }); + + expect(rows).toHaveLength(2); + // No single index serves both arms, so the plan walks the whole table. + // `documents` reports two and hides that entirely; `scanned` is what a + // deployment would be charged and what a read bound has to assert on. + expect(reads.documents).toBe(2); + expect(reads.scanned).toBe(TABLE_SIZE + 2); + }); + + test('the same shape on one index reads only its bucket', async ({ ctx }) => { + for (let i = 0; i < TABLE_SIZE; i += 1) { + await ctx.db.insert('users', { + name: `u${i}`, + email: `u${i}@example.com`, + status: 'filler', + }); + } + await ctx.db.insert('users', { + name: 'a', + email: 'a@example.com', + status: 'zmatch', + }); + await ctx.db.insert('users', { + name: 'b', + email: 'b@example.com', + status: 'ymatch', + }); + + const reads = countDocumentReads(ctx); + const rows = await ctx.orm.query.users.findMany({ + where: { OR: [{ status: 'zmatch' }, { status: 'ymatch' }] }, + }); + + expect(rows).toHaveLength(2); + expect(reads.scanned).toBeLessThanOrEqual(4); + }); + + test('scanned is never smaller than documents', async ({ ctx }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + await ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u9')) + .collect(); + await ctx.db.query('users').take(3); + await ctx.orm.query.users.findMany({ + where: { status: 'bucket' }, + limit: 2, + }); + + expect(reads.scanned).toBeGreaterThanOrEqual(reads.documents); + }); +}); diff --git a/convex/orm/relation-loading.test.ts b/convex/orm/relation-loading.test.ts index deea292d4..5f1d9fd4b 100644 --- a/convex/orm/relation-loading.test.ts +++ b/convex/orm/relation-loading.test.ts @@ -1093,7 +1093,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { }); expect((rows[0] as any).posts).toHaveLength(3); - expect(reads.documents).toBeLessThanOrEqual(5); + expect(reads.scanned).toBeLessThanOrEqual(5); }); test('offset with a relation where skips matches, not scanned rows', async ({ @@ -1407,7 +1407,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { // `by_author` pins authorId, so the rest of the index key is creation // order: the page comes straight off the index instead of collecting all // 40 children and sorting them. - expect(reads.documents).toBeLessThanOrEqual(5); + expect(reads.scanned).toBeLessThanOrEqual(5); }); test('a later compound relation index serves order and bounds reads', async ({ @@ -1436,7 +1436,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { expect((users[0] as any).posts.map((post: any) => post.numLikes)).toEqual( [39, 38, 37] ); - expect(reads.documents).toBeLessThanOrEqual(5); + expect(reads.scanned).toBeLessThanOrEqual(5); }); test('nested with only loads the children that survive the limit', async ({ @@ -1479,7 +1479,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { expect(posts[0].comments).toHaveLength(1); // numLikes is not on `by_author`, so all 10 posts are read โ€” but the 8 // that the limit discards must not each fetch their own comments. - expect(reads.documents).toBeLessThanOrEqual(15); + expect(reads.scanned).toBeLessThanOrEqual(15); }); test('through relation limit bounds the junction read', async ({ ctx }) => { @@ -1504,7 +1504,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { expect((users[0] as any).groups).toHaveLength(3); // 1 user + 3 junction rows + 3 groups. Without the bound this reads all // 30 links and all 30 groups. - expect(reads.documents).toBeLessThanOrEqual(10); + expect(reads.scanned).toBeLessThanOrEqual(10); }); test('through relation bounded read applies offset before limit', async ({ @@ -1533,7 +1533,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { 'group-3', 'group-4', ]); - expect(reads.documents).toBeLessThanOrEqual(14); + expect(reads.scanned).toBeLessThanOrEqual(14); }); test('through relation limit fills the page past filtered targets', async ({ @@ -1568,7 +1568,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { ]); // Refilling still stops at the page: three rounds of three links and // three groups, not all 30 links and all 30 groups. - expect(reads.documents).toBeLessThanOrEqual(25); + expect(reads.scanned).toBeLessThanOrEqual(25); }); test('through relation limit fills the page past dangling links', async ({ @@ -1604,7 +1604,7 @@ describe('M6.5 Phase 3: Relation Filters and Limits', () => { 'group-07', 'group-08', ]); - expect(reads.documents).toBeLessThanOrEqual(25); + expect(reads.scanned).toBeLessThanOrEqual(25); }); }); diff --git a/convex/orm/rls.test.ts b/convex/orm/rls.test.ts index 25fbaa378..67fc93339 100644 --- a/convex/orm/rls.test.ts +++ b/convex/orm/rls.test.ts @@ -349,7 +349,7 @@ describe('RLS', () => { ]); // Counting survivors is not the same as giving up the bound: the scan // stops at the ninth row, it does not collect all 40. - expect(reads.documents).toBeLessThanOrEqual(15); + expect(reads.scanned).toBeLessThanOrEqual(15); }); it('fills a limit past hidden rows when an index serves orderBy', async ({ @@ -383,7 +383,7 @@ describe('RLS', () => { 'label-07', 'label-08', ]); - expect(reads.documents).toBeLessThanOrEqual(15); + expect(reads.scanned).toBeLessThanOrEqual(15); }); it('filters id-only pipeline rows before user callbacks', async ({ ctx }) => { @@ -944,7 +944,7 @@ describe('RLS', () => { expect((rows[0] as any).secrets).toHaveLength(3); // skipRules filters nothing, so the per-parent limit must still be pushed // into the read instead of collecting every child. - expect(reads.documents).toBeLessThan(25); + expect(reads.scanned).toBeLessThan(25); }); it('stops RLS relation reads after the requested visible rows', async ({ @@ -973,6 +973,6 @@ describe('RLS', () => { }); expect((rows[0] as any).secrets).toHaveLength(3); - expect(reads.documents).toBeLessThanOrEqual(5); + expect(reads.scanned).toBeLessThanOrEqual(5); }); }); diff --git a/convex/orm/where-filtering.test.ts b/convex/orm/where-filtering.test.ts index 076361501..0a8ee10d4 100644 --- a/convex/orm/where-filtering.test.ts +++ b/convex/orm/where-filtering.test.ts @@ -1301,9 +1301,16 @@ describe('M4 Where Filtering - Type Safety', () => { }); }); +// These bounds are only worth anything if a degraded plan blows through them, +// so the table is seeded far past every bound: a plan that falls back to a +// table scan reads three hundred documents and cannot squeeze under eight. +// `scanned`, not `documents`, is what makes that visible โ€” a `.filter()` drops +// the rows it rejected before they reach any result. +const READ_BOUND_SEED = 300; + describe('M4: Read bounds', () => { test('in + limit reads a bounded window per probe', async ({ ctx }) => { - for (let i = 0; i < 60; i += 1) { + for (let i = 0; i < READ_BOUND_SEED; i += 1) { await ctx.db.insert('users', { name: `u${i}`, email: `u${i}@example.com`, @@ -1318,17 +1325,18 @@ describe('M4: Read bounds', () => { }); expect(rows).toHaveLength(2); - expect(reads.documents).toBeLessThanOrEqual(8); + // One bounded window per probe. A table scan would be three hundred. + expect(reads.scanned).toBeLessThanOrEqual(8); }); test('in under AND uses the index instead of scanning the table', async ({ ctx, }) => { - for (let i = 0; i < 60; i += 1) { + for (let i = 0; i < READ_BOUND_SEED; i += 1) { await ctx.db.insert('users', { name: `u${i}`, email: `u${i}@example.com`, - status: i >= 57 ? 'zmatch' : 'other', + status: i >= READ_BOUND_SEED - 3 ? 'zmatch' : 'other', }); } @@ -1339,7 +1347,9 @@ describe('M4: Read bounds', () => { }); expect(rows).toHaveLength(2); - // The `zmatch` bucket is three rows; scanning the table would be sixty. - expect(reads.documents).toBeLessThanOrEqual(6); + // `startsWith` cannot ride the index, so the probe reads its whole bucket: + // three rows. Scanning the table would be three hundred, and every one of + // those two hundred and ninety seven rejects is a document Convex bills. + expect(reads.scanned).toBeLessThanOrEqual(6); }); }); diff --git a/convex/setup.testing.ts b/convex/setup.testing.ts index 97c172d2c..31939fb4b 100644 --- a/convex/setup.testing.ts +++ b/convex/setup.testing.ts @@ -118,37 +118,248 @@ export function convexTest>( return wrapConvexTestDateReturns(baseConvexTest(schema, convexModules)); } +export type DocumentReadCounts = { + /** + * Documents handed back to the caller. + * + * This is *not* what a deployment bills. A `.filter()` rejects rows after + * Convex has already read them, and those rows never reach any result, so + * `documents` cannot see them. Assert on `scanned` for read bounds. + */ + documents: number; + /** + * Documents Convex walked, including the ones a `.filter()` rejected. + * + * This is the number a deployment bills, so it is the one that fails when an + * index-backed plan silently degrades into a table scan. + */ + scanned: number; +}; + +/** The serialized plan a `QueryImpl` is holding before it is consumed. */ +type QueryPlan = { + source: Record; + operators: { filter?: unknown; limit?: number }[]; +}; + +/** + * The plan `target` will execute, or `null` when it cannot carry a `.filter()`. + * + * `db.query(table)` is a `QueryInitializerImpl` with no plan yet; every one of + * its terminals delegates to an unfiltered full table scan, so there is nothing + * to account for. A consumed or executing query is likewise past the point + * where the plan can be read, and its own call is about to throw. + */ +const readQueryPlan = (target: any): QueryPlan | null => { + const state = target?.state; + if (!state || state.type !== 'preparing') { + return null; + } + + const plan = state.query; + if (!plan?.source || !Array.isArray(plan.operators)) { + throw new Error( + 'countDocumentReads: unrecognized Convex query plan. Scan counting ' + + 'replays `QueryImpl.state.query` without its `.filter()`; teach it ' + + 'this shape rather than letting read bounds under-report.' + ); + } + + return plan as QueryPlan; +}; + +const planHasFilter = (plan: QueryPlan | null) => + plan !== null && plan.operators.some((operator) => 'filter' in operator); + +const planLimit = (plan: QueryPlan | null) => { + for (const operator of plan?.operators ?? []) { + if (typeof operator.limit === 'number') { + return operator.limit; + } + } + return Number.POSITIVE_INFINITY; +}; + +/** + * Replay `plan`'s source with every operator dropped. + * + * The result is the sequence of documents Convex walks to answer the real + * query: `.filter()` and `.limit()` decide which of them survive, never which + * of them are read. Running it through the engine rather than re-evaluating the + * predicate here keeps the two passes in the same order, so the filtered result + * is always a subsequence of this one. + */ +const scanSequence = async (target: any, plan: QueryPlan): Promise => { + const QueryConstructor = target?.constructor; + if (typeof QueryConstructor !== 'function') { + throw new Error( + 'countDocumentReads: cannot replay a Convex query without its class.' + ); + } + + const replay = new QueryConstructor({ + source: { ...plan.source }, + operators: [], + }); + const rows: any[] = []; + for await (const row of replay) { + rows.push(row); + } + return rows; +}; + +const positionOfId = (sequence: any[], id: unknown, from = 0) => { + if (id === undefined || id === null) { + return -1; + } + + const target = String(id); + for (let index = from; index < sequence.length; index += 1) { + if (String(sequence[index]?._id) === target) { + return index; + } + } + return -1; +}; + /** * Count documents pulled out of `ctx.db` from this point on. * * Read bounds are part of the ORM's contract, and several of them are invisible * in the returned rows: a query that reads a table twice and a query that reads * it once can return exactly the same page. Call this before the query under - * test and assert on `reads.documents`. + * test. + * + * Assert on `reads.scanned`. `reads.documents` only sees rows that survived, + * which makes it blind to exactly the regression these bounds exist to catch: a + * cross-index `OR` that walks the whole table and returns two rows counts two + * documents and three hundred scans. */ -export function countDocumentReads(ctx: { db: GenericDatabaseWriter }): { - documents: number; -} { - const reads = { documents: 0 }; +export function countDocumentReads(ctx: { + db: GenericDatabaseWriter; +}): DocumentReadCounts { + const reads: DocumentReadCounts = { documents: 0, scanned: 0 }; const baseQuery = ctx.db.query.bind(ctx.db); const baseGet = ctx.db.get.bind(ctx.db); + /** + * Account for a terminal that read up to `limit` matches. + * + * Returning fewer than `limit` means the source ran out, so everything in + * range was read. Otherwise Convex stopped at the last match it returned, and + * the scan is that row's position in the unfiltered sequence. + */ + const accountRows = async ( + target: any, + plan: QueryPlan | null, + limit: number, + rows: any[] + ) => { + reads.documents += rows.length; + + if (!planHasFilter(plan)) { + reads.scanned += rows.length; + return; + } + + const sequence = await scanSequence(target, plan as QueryPlan); + if (rows.length < limit) { + reads.scanned += sequence.length; + return; + } + + const last = positionOfId(sequence, rows.at(-1)?._id); + reads.scanned += last === -1 ? sequence.length : last + 1; + }; + + const accountPage = async ( + target: any, + plan: QueryPlan | null, + options: { cursor?: string | null } | undefined, + result: any + ) => { + const rows: any[] = result?.page ?? []; + reads.documents += rows.length; + + if (!planHasFilter(plan)) { + reads.scanned += rows.length; + return; + } + + const sequence = await scanSequence(target, plan as QueryPlan); + // A cursor names the last row of the previous page, so this page starts + // reading right after it. + const start = positionOfId(sequence, options?.cursor ?? null) + 1; + const remaining = Math.max(0, sequence.length - start); + + if (result?.isDone || rows.length === 0) { + reads.scanned += remaining; + return; + } + + const last = positionOfId(sequence, rows.at(-1)?._id, start); + reads.scanned += last === -1 ? remaining : last + 1 - start; + }; + + const wrapIterator = (target: any) => { + // The plan has to be read before iteration begins, because starting the + // query closes it for inspection. + const plan = readQueryPlan(target); + const filtered = planHasFilter(plan); + const limit = planLimit(plan); + const iterator = target[Symbol.asyncIterator](); + let sequence: any[] | null = null; + let cursor = 0; + let matched = 0; + + return { + async next() { + const result = await iterator.next(); + + if (result.done) { + // Same rule as a bounded terminal: running out before the limit is + // reached means the rest of the range was read, while a satisfied + // limit is what stopped the walk. + if (filtered && matched < limit) { + sequence ??= await scanSequence(target, plan as QueryPlan); + reads.scanned += sequence.length - cursor; + cursor = sequence.length; + } + return result; + } + + reads.documents += 1; + matched += 1; + if (!filtered) { + reads.scanned += 1; + return result; + } + + sequence ??= await scanSequence(target, plan as QueryPlan); + const position = positionOfId(sequence, result.value?._id, cursor); + const reached = position === -1 ? cursor : position + 1; + reads.scanned += reached - cursor; + cursor = reached; + return result; + }, + // Abandoning the loop early has to close the underlying query, exactly as + // it would without this proxy. + async return(value?: unknown) { + return typeof iterator.return === 'function' + ? await iterator.return(value) + : { done: true as const, value: undefined }; + }, + [Symbol.asyncIterator]() { + return this; + }, + }; + }; + const wrap = (query: object): any => new Proxy(query, { get(target: any, property) { if (property === Symbol.asyncIterator) { - return () => { - const iterator = target[Symbol.asyncIterator](); - return { - async next() { - const result = await iterator.next(); - if (!result.done) { - reads.documents += 1; - } - return result; - }, - }; - }; + return () => wrapIterator(target); } const value = target[property]; @@ -157,21 +368,45 @@ export function countDocumentReads(ctx: { db: GenericDatabaseWriter }): { } return (...args: unknown[]) => { + const plan = readQueryPlan(target); const result = value.apply(target, args); - if (property === 'collect' || property === 'take') { - return Promise.resolve(result).then((rows: unknown[]) => { - reads.documents += rows.length; + + if (property === 'collect') { + return Promise.resolve(result).then(async (rows: any[]) => { + await accountRows(target, plan, planLimit(plan), rows); return rows; }); } - if (property === 'first' || property === 'unique') { - return Promise.resolve(result).then((row: unknown) => { - if (row) { - reads.documents += 1; - } + if (property === 'take') { + const limit = + typeof args[0] === 'number' ? args[0] : Number.POSITIVE_INFINITY; + return Promise.resolve(result).then(async (rows: any[]) => { + await accountRows(target, plan, limit, rows); + return rows; + }); + } + if (property === 'first') { + return Promise.resolve(result).then(async (row: any) => { + await accountRows(target, plan, 1, row ? [row] : []); return row; }); } + if (property === 'unique') { + // `unique()` is `take(2)`: it keeps reading until a second match + // proves the result ambiguous, so a call that returns at all read + // the whole range. + return Promise.resolve(result).then(async (row: any) => { + await accountRows(target, plan, 2, row ? [row] : []); + return row; + }); + } + if (property === 'paginate') { + const options = args[0] as { cursor?: string | null } | undefined; + return Promise.resolve(result).then(async (page: any) => { + await accountPage(target, plan, options, page); + return page; + }); + } if (result && typeof (result as any).then === 'function') { return result; } @@ -188,6 +423,7 @@ export function countDocumentReads(ctx: { db: GenericDatabaseWriter }): { const row = await baseGet(id as never); if (row) { reads.documents += 1; + reads.scanned += 1; } return row; }; diff --git a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md new file mode 100644 index 000000000..072712bad --- /dev/null +++ b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md @@ -0,0 +1,449 @@ +# 384 scan-aware read counting for filter paths + +Objective: +Make `countDocumentReads` see documents Convex read and a `.filter()` rejected, so read-bound assertions fail when an index-backed plan degrades into a table scan. + +Goal plan: +docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md + +Template: +docs/plans/templates/task.md + +Primary template: +docs/plans/templates/task.md + +Applied packs: +- none + +Task source: +- type: GitHub issue +- id / link: #384 โ€” https://github.com/udecode/kitcn/issues/384 +- title: Testing: `countDocumentReads` counts rows returned, not rows scanned behind `.filter()` +- acceptance criteria: + 1. A scan-counting mode exists alongside the document counter (`{ documents, scanned }`). + 2. The cross-index `OR` scan defect becomes expressible as a regression test. + 3. The `M4: Read bounds` assertions are re-seeded at a larger N so they fail if the plan degrades. +- caveats: no user-facing package change; `convex/` is repo-internal test infrastructure. +- likely files: `convex/setup.testing.ts`, `convex/orm/where-filtering.test.ts`, every `countDocumentReads` call site. +- browser surface: none. +- root-cause layer: test harness proxy. + +Timed checkpoint: +- requested duration: N/A โ€” no duration requested. + +Completion threshold: +- `countDocumentReads` returns `{ documents, scanned }` and `scanned` counts rows + a Convex `.filter()` rejected, with early stop modelled for bounded terminals. +- A harness contract suite proves it, and fails against the old harness. +- `M4: Read bounds` asserts on `scanned` at N=300. +- Every pre-existing `countDocumentReads` assertion still passes. +- Task closure is legal only when the source-of-truth acceptance criteria are + satisfied or explicitly narrowed, required verification evidence is recorded, + code-review and release-artifact gates are closed when applicable, verified + code changes are committed and PR'd unless explicitly declined or blocked, + task-style PR body sync is complete or marked N/A with reason, + GitHub issue/PR sync is complete or marked N/A with reason, and + `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md` passes. + +Verification surface: +- `npx vitest run --project integration` (full integration suite, cwd repo root). +- `npx vitest run convex/orm/read-counting.test.ts --project integration`. +- Red-proof: same suite with `convex/setup.testing.ts` stashed to HEAD. +- `bun run test:bun`, `bun typecheck`, `bun lint`. + +Constraints: +- Preserve existing user-facing behavior outside the task scope. +- Prefer the durable ownership boundary over caller-by-caller patches. +- When a GitHub PR is in scope, this plan owns exactly one PR. A coordinating + batch plan must link a separate task plan for every PR an agent processes. +- Verified code changes must be committed and PR'd because the task skill + requires that path unless the user explicitly says not to, the work has no + local patch, or a real blocker is recorded. +- The absence of a separate "open a PR" sentence from the user is not a valid + N/A reason for verified code-changing task work. +- A PR created by this task must use the PR #270 emoji task-style PR body + contract below, not a generic summary/body from a git helper skill. +- A task-run PR body must include + `๐Ÿงญ Task plan: docs/plans/.md`; the plan must exist at the PR head and + identify the exact PR before autoclosure. +- Do not add broad ceremony when the task is trivial or docs-only. + +Boundaries: +- Source of truth: GitHub issue #384. +- Allowed edit scope: `convex/setup.testing.ts`, `convex/orm/*.test.ts`, this plan. +- Browser surface: N/A โ€” no rendered output. +- GitHub issue sync: N/A โ€” user declined PR; no PR exists to reference. +- Non-goals: fixing the cross-index `OR` plan itself (this task makes it + measurable); renaming `documents` across 30 call sites; touching `packages/`. + +Output budget strategy: +- Test output filtered through `grep -E "Tests |Test Files|ร—|AssertionError"`. +- Source reads scoped with `sed -n` ranges and targeted `grep -n`, never whole + 8976-line files. + +Blocked condition: +- None encountered. + +Task state: +- task_type: testing / harness bug +- task_complexity: non-trivial +- current_phase: closeout +- current_phase_status: complete +- next_phase: final response +- goal_status: complete + +Current verdict: +- verdict: complete +- confidence: 95-100% +- next owner: task +- reason: every source-listed case has direct proof at the owning layer, and the + new suite is red against the old harness. + +Implementation readiness: +- verdict: ready +- exact owner: `countDocumentReads` in `convex/setup.testing.ts` +- contradiction status: none +- source-listed cases complete: yes + +Pre-solution issue challenge: +- reporter claim: `countDocumentReads` reports 2 for a cross-index `OR` that + walks 302 documents. +- suggested diagnosis or fix: (a) wrap `.filter()` and count predicate + invocations, exposing `{ documents, scanned }`, or (b) assert plan shape. +- repro ladder: + - tests / source-level repro: reproduced exactly. A scratch test seeded 302 + rows, ran `findMany({ where: { OR: [{status:'zmatch'},{age:99}] } })`, got + 2 rows and `{"documents":2}`. + - repo-owned automated integration proof: `convex/orm/read-counting.test.ts`. + - Browser plugin: N/A โ€” no browser surface. + - screenshot / visual proof: N/A. +- reproduction verdict: valid +- validity verdict: partially valid โ€” the defect is real and exactly as + described, but the suggested mechanism (a) is not implementable. +- what was wrong in the proposed path: `.filter()`'s callback is invoked + **once per query**, not once per document + (`convex/dist/esm/server/impl/query_impl.js:168` calls + `predicate(filterBuilderImpl)` to build a serialized expression). Counting + predicate invocations would count 1, always. Option (b) is also already + available: `where-clause-compiler.test.ts` asserts `result.strategy` in 19 + places today, so it was never the missing capability. +- best long-term fix boundary: read the serialized plan off + `QueryImpl.state.query` before the terminal consumes it, replay the same + source with no operators, and count the walk. This uses the engine for both + passes rather than re-implementing `evaluateFilter`, so the filtered result is + always a subsequence of the scan sequence. +- hard-stop decision: proceed โ€” reproduced at the source layer before any edit. + +Completion rule: +- Do not call `update_goal(status: complete)` while any required checklist item + remains unchecked. If an item does not apply, check it and add `N/A: `. +- Do not call `update_goal(status: complete)` until every completion threshold + above is satisfied, final handoff evidence is recorded, and + `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md` passes. +- Do not create hook state for this goal. This file plus the active goal are the + durable state. + +Start Gates: +| Gate | Applies | Evidence | +|------|---------|----------| +| Timed checkpoint parsed | no | No duration requested | +| Walkthrough baseline for possible UI change | no | N/A: test harness only, no UI or rendered output | +| Skill analysis before edits | yes | task + autogoal; testing policy applied inline, no extra skills earned their keep | +| Active goal checked or created | yes | this plan | +| Source of truth read before edits | yes | gh issue view 384 + attachment read first | +| Exact per-PR task ownership | no | N/A: user explicitly forbids PR creation | +| GitHub comments and attachments read | yes | issue has 0 comments; attachment read | +| Video transcript evidence required | no | N/A: no video evidence | +| Pre-solution issue challenge required | yes | recorded above: partially valid | +| Reproduction verdict before implementation | yes | valid; scratch probe reported documents 2 over 302 rows | +| Repro escalation ladder selected | yes | source-level repro sufficed; browser N/A | +| Suggested fix reviewed against durable boundary | yes | issue option (a) disproven, option (b) already present | +| `docs/solutions` checked for non-trivial existing-code work | no | N/A: no docs/solutions directory in repo | +| TDD decision before behavior change or bug fix | yes | red-proof captured by stashing the harness to HEAD | +| Branch decision for code-changing task | yes | already on issue-384, dedicated to this issue | +| Release artifact decision | no | N/A: root package is private, packages/ untouched | +| Browser tool decision for browser surface | no | N/A: no browser surface | +| Commit / PR expectation decision | no | N/A: user preference explicitly forbids commit/PR | +| Task-style PR body decision | no | N/A: no PR | +| Task-plan PR body evidence | no | N/A: no PR | +| GitHub issue sync expectation decision | no | N/A: no PR to reference | +| Output budget strategy recorded | yes | recorded above | + +Work Checklist: +- [x] If a duration was requested, it is recorded as minimum active work unless + explicitly marked hard stop; when no better metric exists, initial and + final confidence scores are recorded. +- [x] Objective includes outcome, completion threshold, verification surface, + constraints, boundaries, and blocked condition. +- [x] Task source classified with source type, id/link, title, task type, + acceptance criteria, caveats, likely files/routes/packages, browser + surface, and root-cause layer. +- [x] Every GitHub PR in scope has its own task plan. This plan owns one exact + PR, owns a not-yet-created PR slice, or records N/A because no PR is in + scope; a batch plan is not used as a substitute. +- [x] Required video or screen-recording evidence is cached/read as normalized + `` XML, or marked N/A with reason. +- [x] For public GitHub bug reports, behavior claims, technical diagnoses, or + suggested fixes, reporter claims are challenged before implementation + with a recorded verdict: `valid`, `not reproduced`, `invalid`, + `wont-fix`, `partially valid`, or `platform limitation`. Feature, docs, + support, or cleanup requests with no bug claim may mark reproduction + `N/A` with reason. +- [x] Repro escalation ladder followed for bug/behavior claims: focused + test/source-level repro first when applicable; existing repo-owned + automated browser or integration proof next when available and useful as + executable coverage; the repo-approved Browser tool next when tests or + automation cannot reproduce or cannot model the surface honestly; + screenshot or explicit visual-proof waiver when visual/native state + matters. +- [x] Hard-stop rule followed for bug/behavior claims: no code when the issue + is not reproduced, invalid, or won't-fix; partial validity pivots to the + best long-term fix and records what was wrong or incomplete in the + issue's proposed path. +- [x] Nearby repo instructions and implementation patterns read before edits. +- [x] Source-listed case matrix is complete and every contradiction has an + owner, harness, and verdict before mutation. +- [x] Readiness is classified `ready`, `repair-source`, `major`, `blocked`, or + `invalid` with evidence. +- [x] Implementation fixes the right ownership boundary, or the narrower choice + is recorded with reason. +- [x] Release artifact requirement recorded: active changeset, new changeset, or + N/A with reason. +- [x] Final handoff shape decided: bug/feature/testing/batch/review/GitHub + requirements, PR body sync, and issue sync when applicable. +- [x] Commit/PR handling recorded for code-changing work: commit and PR + completed, no local patch, user explicitly declined, or blocker recorded. + "User did not separately ask for a PR" is not a valid blocker. +- [x] PR body shape recorded: PR #270 emoji task-style body used, N/A reason + recorded, or blocker recorded. +- [x] PR task evidence recorded: body includes `๐Ÿงญ Task plan: ...`, the plan + exists at the PR head, and it identifies the exact PR before autoclosure. +- [x] Branch handling recorded for code-changing work: dedicated branch used, + new branch needed, or N/A with reason. +- [x] Local-env-rot retry policy recorded for any surprising repo-wide failure: + reinstall/rerun evidence or N/A with reason. +- [x] Workspace authority recorded: every proof command names the cwd/tool that + owns the changed behavior. +- [x] Output budget discipline recorded and followed: broad searches are + scoped, capped, counted, or artifacted instead of streamed into goal + context. +- [x] High-risk note recorded for public API, runtime, package-boundary, + browser behavior, agent-action, or command-contract changes, or marked + N/A with reason. +- [x] Review/autoreview target selected from actual diff state for non-trivial + implementation work, or marked N/A with reason. +- [x] Agent-native review decision recorded for `.agents/**`, `.claude/**`, + `.codex/**`, skills, hooks, commands, prompts, or user-action tooling. + +Completion Gates: +| Gate | Applies | Required action | Evidence | +|------|---------|-----------------|----------| +| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | vitest integration suite + bun test + typecheck + lint, all recorded | +| Exact per-PR task ownership | no | N/A: user explicitly forbids PR creation | No PR in scope | +| Pre-solution issue challenge verdict | yes | Record reporter claim, suggested fix, repro verdict, validity verdict, durable boundary, and hard-stop/pivot decision before implementation | partially valid; recorded with the disproof of option (a) | +| Repro escalation ladder | yes | For bug/behavior claims, record test/source-level, automated browser/integration, Browser, and screenshot/visual-proof outcomes or N/A/blocker reasons before `not reproduced` | source-level repro reproduced it; browser N/A | +| Bug reproduced before fix | yes | Record failing test/repro or N/A with reason | scratch probe: documents 2 over 302 rows, before any edit | +| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | 14/14 read-counting.test.ts; 15 red at HEAD | +| TypeScript or typed config changed | yes | Run relevant typecheck | bun typecheck: 5 tasks successful | +| Package exports or file layout changed | no | Run the relevant package build before final verification and keep generated updates | N/A: packages/ untouched | +| Package manifests, lockfile, or install graph changed | no | Run `bun install` and relevant package checks | N/A: no manifest change | +| Agent rules or skills changed | no | Run `bun install` and verify generated skill sync | N/A: .agents untouched | +| Workspace authority proof | yes | Run verification in the owning repo/package/app/route/tool and record cwd; do not count the wrong workspace as proof | all commands run in /Users/mikey/conductor/workspaces/kitcn/moab, which owns convex/ | +| Browser surface changed | no | Capture Browser Use proof or record explicit waiver/blocker | N/A: no browser surface | +| Browser final proof | no | Attach screenshot or exact browser verification caveat when browser proof applies | N/A: no browser surface | +| UI walkthrough | no | If UI or rendered output changed, run `.agents/skills/walkthrough/SKILL.md` after final proof and show annotated images in the final handoff; otherwise record N/A | N/A: no UI or rendered output changed | +| Scaffold or fixture output changed | no | Run `bun run fixtures:sync` and `bun run fixtures:check`, or record N/A | N/A: no init -t template or scaffold source touched | +| Package behavior or public API changed | no | Add a changeset or record why no changeset applies | N/A: root package private, packages/ untouched, no changeset needed | +| Docs and kitcn skill sync changed | no | Keep `www/**` and `packages/kitcn/skills/kitcn/**` in sync, or record N/A | N/A: no www/ or skills docs touched | +| Docs or content changed | no | For docs-heavy work, use `--template docs`; for incidental docs, verify source-backed claims, links, examples, and rendered output or record N/A | N/A: only this plan | +| High-risk mini gate | yes | For public API/runtime/package-boundary/browser/agent-action/command-contract changes, record realistic failure mode, proof plan, and why the chosen boundary is right; otherwise N/A | see Open risks: internal convex shape read, loud throw on drift | +| Agent-native review for agent/tooling changes | no | For `.agents/**`, `.claude/**`, `.codex/**`, skills, hooks, commands, prompts, or user-action tooling, load `.agents/skills/agent-native-reviewer/SKILL.md` and close accepted/actionable findings, or record N/A | N/A: no .agents/.claude/.codex/skill/hook/command change | +| Local install corruption suspected | no | Run `bun install` once, rerun the exact failing command, or record N/A | N/A: no corruption signals | +| Commit created | no | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | N/A: user preference explicitly forbids commit/PR | +| PR create or update | no | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | N/A: user preference explicitly forbids commit/PR | +| Task-style PR body verified | no | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must use the PR #270 emoji format: `๐Ÿ› Fixes ...`, `๐ŸŸข 95-100% confidence`, `Phase / ๐Ÿงช Tests / ๐ŸŒ Browser` table, and bold emoji Outcome/Caveat/Design/Verified sections | N/A: no PR | +| PR task evidence verified | no | Verify body plan line, plan at PR head, and exact PR ownership | N/A: no PR | +| PR proof image hosting | no | If PR body needs browser proof, replace local image paths with hosted GitHub URLs or record N/A | N/A: no PR | +| GitHub issue sync-back | no | Post concise issue sync after PR exists, or record N/A/blocker | N/A: no PR to reference; user forbids PR | +| Final handoff contract | yes | Fill the final handoff fields below with exact PR/issue/confidence/tests/browser/outcome/caveats/design/verification content or N/A reason | filled above | +| Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint: no fixes applied | +| Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | all test output grep-filtered; source reads range-scoped | +| Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | autoreview --mode local, no accepted findings | +| Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md` | see below | + +Phase / pass table: +| Phase | Status | Evidence | Next | +|-------|--------|----------|------| +| Intake and source read | complete | issue #384 fetched, convex-test + query_impl read | implementation | +| Implementation | complete | scan counting + 13-case suite + M4 re-seed | verification | +| Verification | complete | 13/13 new, 174 pre-existing, 1288 bun, typecheck, lint | closeout | +| Commit / PR / GitHub sync | N/A | user preference explicitly forbids PR | closeout | +| Closeout | complete | autoreview + check-complete | final response | + +Findings: +- `take`, `first`, and `unique` are all `limit(n).collect()` over the async + iterator (`query_impl.js:236-255`), so one accounting rule covers them. +- convex-test's `_evaluateQuery` applies source -> `.filter()` -> sort -> limit + (`convex-test/dist/index.js:271-343`). The sort key is the index fields plus + `_creationTime` and `_id`, and `_creationTime` is strictly increasing + (`index.js:81`), so a filtered run is always a subsequence of an unfiltered + one. That is what makes the replay comparison sound. +- convex-test applies `limit` *after* a full scan and sort, so the emulator + cannot model early stop on its own; the replay does it explicitly. +- Every existing `countDocumentReads` assertion already held on `scanned`, so + no ORM plan was actually degraded at those seeds; the assertions were just + unable to prove it. +- `btree.vitest.ts` fails on roughly 1 run in 8 on a random fast-check seed + (measured: 1 of 8 runs). It has zero references to `countDocumentReads`. + +Decisions and tradeoffs: +- Kept `documents` rather than renaming it, so all 30 call sites stayed + readable, and pointed the JSDoc at the trap instead. Every read-bound + assertion was moved to `scanned`, which is the metric they always meant. +- Modelled early stop for `take`/`first`/paginate instead of charging the whole + range. Overstating a bound would fail plans that real Convex would not bill. +- Chose an explicit throw over a silent fallback when the plan shape is + unrecognized: silently under-reporting reads is precisely the defect in #384. +- Re-seeded only the `M4: Read bounds` block that the issue named. + `ordering.test.ts` has the same shape at N=60, but at a 7.5x margin on + `scanned` it is already non-vacuous, and widening it is scope the issue did + not ask for. + +Implementation notes: +- `convex/setup.testing.ts`: `countDocumentReads` now returns + `DocumentReadCounts { documents, scanned }`. Added `readQueryPlan`, + `planHasFilter`, `planLimit`, `scanSequence`, `positionOfId`, `accountRows`, + `accountPage`, `wrapIterator`, and `paginate` interception. +- The wrapped iterator now forwards `return()`, so breaking out of a + `for await` closes the underlying query exactly as it would unproxied. +- Scan replay only runs when the plan carries a Convex `.filter()`, so stream + paths and unfiltered ranges cost nothing extra. + +Review fixes: +- `autoreview --mode local --engine claude` pass 1: clean, no accepted findings + ("patch is correct", 0.75). Sub-threshold note accepted anyway: the wrapped + iterator's `done` branch charged the whole remaining range even when a + serialized `limit` operator was what ended iteration, while `collect()` + already honored `planLimit`. Fixed by tracking `matched` and draining only + when `matched < limit`, which is the same rule `accountRows` uses. Pinned by + a 14th case; without the guard it reports `expected 300 to be 3`. +- `autoreview --mode local --engine claude` pass 2 after the fix: clean, + "No landing blockers found" (0.72). + +Error attempts: +| Error / failed attempt | Count | Next different move | Resolution | +|------------------------|-------|---------------------|------------| +| None yet | 0 | | | + +Verification evidence: +- cwd for all commands: `/Users/mikey/conductor/workspaces/kitcn/moab`. +- `npx vitest run convex/orm/read-counting.test.ts --project integration` + -> 14 passed. +- `npx vitest run --project integration` -> 716 passed, 13 skipped, 1 failed + (`btree.vitest.ts`, pre-existing seed flake; 7 of 8 standalone reruns pass, + 0 references to `countDocumentReads`). +- Red-proof with `convex/setup.testing.ts` stashed to HEAD -> 15 failed + (13 new + 2 re-seeded M4), including `expected undefined to be 302`. +- The 7 pre-existing `countDocumentReads` suites -> 174 passed, 1 skipped, + both before and after switching every assertion to `scanned`. +- `bun run test:bun` -> 1285 pass, 1 fail (`package-intent.test.ts`, + pre-existing `npm pack` hook timeout; fails at HEAD with the tree stashed, + 0 references to `countDocumentReads`). Earlier run in this session: 1288 + pass, 0 fail. +- `npx vitest run --project integration` after the review fix -> 718 passed, + 13 skipped, 0 failed. +- `bun typecheck` -> 5 tasks successful. +- `bun lint` -> no fixes applied. + +Source-listed case matrix: +| Case | Source claim | Harness | Before | Expected after | Evidence | Status | +| --- | --- | --- | --- | --- | --- | --- | +| Cross-index `OR` scan is invisible | walks 302 to return 2, harness reports 2 | `read-counting.test.ts` "an OR across two indexed fields" | `{documents:2}` | `documents:2, scanned:302` | passes; red at HEAD | done | +| `.filter()` paths are blind | rows rejected never surface | 7 filter cases in `read-counting.test.ts` | no `scanned` field | `scanned` charges rejects | 13/13 pass | done | +| Serialized `limit` ends iteration | (autoreview) | "a filtered iterator run to done stops on a satisfied limit" | charged 300 | `scanned: 3` | passes; 300 without the guard | done | +| Stream paths are already fine | `filterWith` pulls every scanned row | "a JavaScript stream filter is already fully visible" | 300 | `scanned == documents == 300` | passes | done | +| `M4: Read bounds` overstates coverage | bucket is 3 rows, would pass under a scan | `where-filtering.test.ts` M4 block | `documents <= 6` at N=60 | `scanned <= 6` at N=300 | passes | done | +| Existing invariants must not regress | โ€” | 7 suites, 30 assertions | 174 pass | 174 pass on `scanned` | passes | done | + +Final handoff contract: +- Commit line: N/A โ€” user preference explicitly forbids commit/PR + ("Do not create PR under any circumstances, unless user prompts to"). +- PR line: N/A โ€” same explicit user decline. +- Issue line: N/A โ€” no PR exists to reference in an issue sync. +- Confidence line: 95-100%. +- Flow table: + - Reproduced: tests red at HEAD (15 failures), browser N/A + - Verified: tests green (14 new + 174 pre-existing + 1288 bun), browser N/A +- Browser check: N/A โ€” test harness change, no rendered output. +- Outcome: `countDocumentReads` now reports `{ documents, scanned }`; `scanned` + charges for rows a Convex `.filter()` read and rejected, with early stop + modelled for bounded terminals. +- Caveat: `btree.vitest.ts` (random seed) and `package-intent.test.ts` + (`npm pack` hook timeout) are pre-existing flakes unrelated to this diff. The cross-index `OR` plan itself is unchanged โ€” this task makes it + measurable, and it now deserves its own issue. +- Design: + - Chosen boundary: the harness proxy in `convex/setup.testing.ts`, replaying + the serialized plan without its operators. + - Why not quick patch: the issue's own suggestion (count `.filter()` predicate + invocations) is impossible โ€” the callback runs once per query. + - Why not broader change: plan-shape assertions already exist in + `where-clause-compiler.test.ts`, and renaming `documents` would churn 30 + call sites for no additional proof. +- Verified: see Verification evidence. +- PR body verified: N/A โ€” no PR. + +Task-style PR body contract: +- Preserve any existing `` block. If a changeset is + part of the diff and repo policy expects auto release, include that block. +- Use the accepted PR #270 visual format. The body starts with an emoji + issue/fix line, for example `๐Ÿ› Fixes #123` or `๐Ÿ› Fixes โž– N/A`, then + `๐Ÿงญ Task plan: docs/plans/.md`, then an emoji confidence line like + `๐ŸŸข 95-100% confidence`. +- Use this exact table header: `| Phase | ๐Ÿงช Tests | ๐ŸŒ Browser |`. +- Use `Reproduced` and `Verified` rows. Mark passing proof with `๐ŸŸข`, repro or + failing proof with `๐Ÿ”ด`, and non-applicable cells with `โž– N/A`. +- Use bold emoji section headings: `**โœ… Outcome**`, `**โš ๏ธ Caveat**`, + `**๐Ÿ—๏ธ Design**`, and `**๐Ÿงช Verified**`. +- Never include a line that links to the current PR itself. The current PR URL + belongs in the final response, not in its own description. +- Do not replace this with a generic `Summary` / `Verification` PR body, an + adaptive prose body from a git helper skill, plain `## Outcome` sections, or + an unrelated generated badge footer unless the caller or repo template + explicitly asks for it. +- Proof is `gh pr view --json body` output or a concise source-backed summary + of that output. + +Final handoff / sync: +- Commit: N/A โ€” explicit user decline. +- PR: N/A โ€” explicit user decline. +- Issue: N/A โ€” no PR to reference. +- Browser proof: N/A. +- Caveats: pre-existing `btree.vitest.ts` seed flake. + +Timeline: +- 2026-08-21T14:11:24.460Z Task goal plan created. +- 2026-08-21 Defect reproduced at source layer: documents 2, table 302. +- 2026-08-21 `{ documents, scanned }` implemented; harness suite green. +- 2026-08-21 autoreview x2 clean; limit-aware iterator fix + 14th case added. +- 2026-08-21 Red-proof at HEAD: 15 failures. Full gates green except known flake. + +Reboot status: +| Question | Answer | +|----------|--------| +| Where am I? | Intake and source read | +| Where am I going? | Implementation, verification, commit/PR/GitHub sync, closeout | +| What is the goal? | Make read bounds able to see rows a `.filter()` rejected | +| What have I learned? | See Findings | +| What have I done? | See Timeline | + +Open risks: +- Scan replay reads `QueryImpl.state.query` and `target.constructor`, which are + internal to the pinned `convex` version. A shape change throws a named error + rather than under-reporting, so the failure is loud. +- The cross-index `OR` full scan is now measurable but still unfixed. + +Hard closeout guard: +- A local-only final response for verified code-changing work is invalid unless + this plan records an explicit user decline, no local patch, analytical/ + blocked/inconclusive outcome, or a real commit/PR blocker. From fe7f0222957817055c29dab607d0b9468a48df91 Mon Sep 17 00:00:00 2001 From: MikeyZhang75 Date: Sat, 22 Aug 2026 00:12:10 +0800 Subject: [PATCH 2/5] docs: bind scan-counting task plan to PR 399 --- ...an-aware-read-counting-for-filter-paths.md | 51 +++++++++++-------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md index 072712bad..d221be5a7 100644 --- a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md +++ b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md @@ -18,6 +18,7 @@ Applied packs: Task source: - type: GitHub issue - id / link: #384 โ€” https://github.com/udecode/kitcn/issues/384 +- PR: #399 โ€” https://github.com/udecode/kitcn/pull/399 - title: Testing: `countDocumentReads` counts rows returned, not rows scanned behind `.filter()` - acceptance criteria: 1. A scan-counting mode exists alongside the document counter (`{ documents, scanned }`). @@ -71,8 +72,9 @@ Constraints: Boundaries: - Source of truth: GitHub issue #384. - Allowed edit scope: `convex/setup.testing.ts`, `convex/orm/*.test.ts`, this plan. +- Branch: `test/scan-aware-read-counting` (renamed from `issue-384` before first push). - Browser surface: N/A โ€” no rendered output. -- GitHub issue sync: N/A โ€” user declined PR; no PR exists to reference. +- GitHub issue sync: PR #399 opened; `Fixes #384` in the body links it. - Non-goals: fixing the cross-index `OR` plan itself (this task makes it measurable); renaming `documents` across 30 call sites; touching `packages/`. @@ -151,7 +153,7 @@ Start Gates: | Skill analysis before edits | yes | task + autogoal; testing policy applied inline, no extra skills earned their keep | | Active goal checked or created | yes | this plan | | Source of truth read before edits | yes | gh issue view 384 + attachment read first | -| Exact per-PR task ownership | no | N/A: user explicitly forbids PR creation | +| Exact per-PR task ownership | yes | PR #399 โ€” this plan owns exactly that PR | | GitHub comments and attachments read | yes | issue has 0 comments; attachment read | | Video transcript evidence required | no | N/A: no video evidence | | Pre-solution issue challenge required | yes | recorded above: partially valid | @@ -163,10 +165,10 @@ Start Gates: | Branch decision for code-changing task | yes | already on issue-384, dedicated to this issue | | Release artifact decision | no | N/A: root package is private, packages/ untouched | | Browser tool decision for browser surface | no | N/A: no browser surface | -| Commit / PR expectation decision | no | N/A: user preference explicitly forbids commit/PR | -| Task-style PR body decision | no | N/A: no PR | -| Task-plan PR body evidence | no | N/A: no PR | -| GitHub issue sync expectation decision | no | N/A: no PR to reference | +| Commit / PR expectation decision | yes | user requested the PR in a follow-up turn | +| Task-style PR body decision | yes | PR #270 emoji task-style body | +| Task-plan PR body evidence | yes | plan line in body; plan at head names PR #399 | +| GitHub issue sync expectation decision | yes | `Fixes #384` in PR body | | Output budget strategy recorded | yes | recorded above | Work Checklist: @@ -239,7 +241,7 @@ Completion Gates: | Gate | Applies | Required action | Evidence | |------|---------|-----------------|----------| | Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | vitest integration suite + bun test + typecheck + lint, all recorded | -| Exact per-PR task ownership | no | N/A: user explicitly forbids PR creation | No PR in scope | +| Exact per-PR task ownership | yes | This plan owns exactly PR #399 | PR #399 | | Pre-solution issue challenge verdict | yes | Record reporter claim, suggested fix, repro verdict, validity verdict, durable boundary, and hard-stop/pivot decision before implementation | partially valid; recorded with the disproof of option (a) | | Repro escalation ladder | yes | For bug/behavior claims, record test/source-level, automated browser/integration, Browser, and screenshot/visual-proof outcomes or N/A/blocker reasons before `not reproduced` | source-level repro reproduced it; browser N/A | | Bug reproduced before fix | yes | Record failing test/repro or N/A with reason | scratch probe: documents 2 over 302 rows, before any edit | @@ -259,12 +261,12 @@ Completion Gates: | High-risk mini gate | yes | For public API/runtime/package-boundary/browser/agent-action/command-contract changes, record realistic failure mode, proof plan, and why the chosen boundary is right; otherwise N/A | see Open risks: internal convex shape read, loud throw on drift | | Agent-native review for agent/tooling changes | no | For `.agents/**`, `.claude/**`, `.codex/**`, skills, hooks, commands, prompts, or user-action tooling, load `.agents/skills/agent-native-reviewer/SKILL.md` and close accepted/actionable findings, or record N/A | N/A: no .agents/.claude/.codex/skill/hook/command change | | Local install corruption suspected | no | Run `bun install` once, rerun the exact failing command, or record N/A | N/A: no corruption signals | -| Commit created | no | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | N/A: user preference explicitly forbids commit/PR | -| PR create or update | no | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | N/A: user preference explicitly forbids commit/PR | -| Task-style PR body verified | no | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must use the PR #270 emoji format: `๐Ÿ› Fixes ...`, `๐ŸŸข 95-100% confidence`, `Phase / ๐Ÿงช Tests / ๐ŸŒ Browser` table, and bold emoji Outcome/Caveat/Design/Verified sections | N/A: no PR | -| PR task evidence verified | no | Verify body plan line, plan at PR head, and exact PR ownership | N/A: no PR | -| PR proof image hosting | no | If PR body needs browser proof, replace local image paths with hosted GitHub URLs or record N/A | N/A: no PR | -| GitHub issue sync-back | no | Post concise issue sync after PR exists, or record N/A/blocker | N/A: no PR to reference; user forbids PR | +| Commit created | yes | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | feb68240, whole checkout staged | +| PR create or update | yes | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | PR #399 opened; `check:ci` exit 0; `test:runtime` blocked by fixed-port contention | +| Task-style PR body verified | yes | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must use the PR #270 emoji format: `๐Ÿ› Fixes ...`, `๐ŸŸข 95-100% confidence`, `Phase / ๐Ÿงช Tests / ๐ŸŒ Browser` table, and bold emoji Outcome/Caveat/Design/Verified sections | `gh pr view --json body`, PR #270 emoji format, no self-link | +| PR task evidence verified | yes | Verify body plan line, plan at PR head, and exact PR ownership | body names this plan; plan at PR head names PR #399 | +| PR proof image hosting | no | If PR body needs browser proof, replace local image paths with hosted GitHub URLs or record N/A | N/A: no browser proof in body | +| GitHub issue sync-back | yes | Post concise issue sync after PR exists, or record N/A/blocker | `Fixes #384` in PR #399 body | | Final handoff contract | yes | Fill the final handoff fields below with exact PR/issue/confidence/tests/browser/outcome/caveats/design/verification content or N/A reason | filled above | | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint: no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | all test output grep-filtered; source reads range-scoped | @@ -278,7 +280,7 @@ Phase / pass table: | Intake and source read | complete | issue #384 fetched, convex-test + query_impl read | implementation | | Implementation | complete | scan counting + 13-case suite + M4 re-seed | verification | | Verification | complete | 13/13 new, 174 pre-existing, 1288 bun, typecheck, lint | closeout | -| Commit / PR / GitHub sync | N/A | user preference explicitly forbids PR | closeout | +| Commit / PR / GitHub sync | complete | feb68240 pushed; PR #399 opened and bound to this plan | closeout | | Closeout | complete | autoreview + check-complete | final response | Findings: @@ -355,6 +357,14 @@ Verification evidence: 13 skipped, 0 failed. - `bun typecheck` -> 5 tasks successful. - `bun lint` -> no fixes applied. +- `bun run check:ci` -> exit 0 (lint, typecheck, test, test:cli, test:concave, + fixtures:check). `fixtures:check` needed one retry after a bun link `EEXIST` + race, then passed. +- `bun run test:runtime` -> blocked, not failed: `EADDRINUSE 127.0.0.1:3211` + across four attempts, from parallel Conductor workspaces holding that fixed + port. Unreachable from this diff: the commit touches zero files under + `packages/`, and `tooling/scenarios.ts` never references `setup.testing` or + `convex/orm`. CI owns that lane. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -367,10 +377,9 @@ Source-listed case matrix: | Existing invariants must not regress | โ€” | 7 suites, 30 assertions | 174 pass | 174 pass on `scanned` | passes | done | Final handoff contract: -- Commit line: N/A โ€” user preference explicitly forbids commit/PR - ("Do not create PR under any circumstances, unless user prompts to"). -- PR line: N/A โ€” same explicit user decline. -- Issue line: N/A โ€” no PR exists to reference in an issue sync. +- Commit line: `feb68240 test(orm): count documents scanned behind .filter()`. +- PR line: https://github.com/udecode/kitcn/pull/399 (base `main`, head `test/scan-aware-read-counting`). +- Issue line: #384, closed by `Fixes #384` in the PR body. - Confidence line: 95-100%. - Flow table: - Reproduced: tests red at HEAD (15 failures), browser N/A @@ -415,9 +424,9 @@ Task-style PR body contract: of that output. Final handoff / sync: -- Commit: N/A โ€” explicit user decline. -- PR: N/A โ€” explicit user decline. -- Issue: N/A โ€” no PR to reference. +- Commit: feb68240 (plus a follow-up binding this plan to PR #399). +- PR: #399. +- Issue: #384 linked via `Fixes #384`. - Browser proof: N/A. - Caveats: pre-existing `btree.vitest.ts` seed flake. From d2db190d7748a6ef59f6da9e8058c18e94c4a44d Mon Sep 17 00:00:00 2001 From: MikeyZhang75 Date: Sat, 22 Aug 2026 00:20:00 +0800 Subject: [PATCH 3/5] docs: record PR 399 CI proof --- ...2026-08-21-384-scan-aware-read-counting-for-filter-paths.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md index d221be5a7..1bd6a2fc4 100644 --- a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md +++ b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md @@ -364,7 +364,7 @@ Verification evidence: across four attempts, from parallel Conductor workspaces holding that fixed port. Unreachable from this diff: the commit touches zero files under `packages/`, and `tooling/scenarios.ts` never references `setup.testing` or - `convex/orm`. CI owns that lane. + `convex/orm`. Closed by CI: PR #399 CI passed in 6m21s, covering that lane. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -435,6 +435,7 @@ Timeline: - 2026-08-21 Defect reproduced at source layer: documents 2, table 302. - 2026-08-21 `{ documents, scanned }` implemented; harness suite green. - 2026-08-21 autoreview x2 clean; limit-aware iterator fix + 14th case added. +- 2026-08-22 branch renamed to test/scan-aware-read-counting; PR #399 opened; CI green. - 2026-08-21 Red-proof at HEAD: 15 failures. Full gates green except known flake. Reboot status: From b6dc050cfd6912bf6ddd368925ef805233d02734 Mon Sep 17 00:00:00 2001 From: zbeyens Date: Tue, 25 Aug 2026 22:26:10 +0200 Subject: [PATCH 4/5] fix scan-aware pagination accounting --- convex/orm/read-counting.test.ts | 73 ++++++++++++++++++ convex/setup.testing.ts | 76 ++++++++++++++++--- ...an-aware-read-counting-for-filter-paths.md | 21 ++++- fixtures/next-auth/package.json | 2 +- fixtures/next/package.json | 2 +- fixtures/start-auth/package.json | 2 +- fixtures/start/package.json | 2 +- fixtures/vite-auth/package.json | 2 +- fixtures/vite/package.json | 2 +- 9 files changed, 162 insertions(+), 20 deletions(-) diff --git a/convex/orm/read-counting.test.ts b/convex/orm/read-counting.test.ts index 90701201f..e02f9dad7 100644 --- a/convex/orm/read-counting.test.ts +++ b/convex/orm/read-counting.test.ts @@ -189,6 +189,79 @@ describe('countDocumentReads: rows rejected by .filter()', () => { // Twenty one rows had to be read to fill a two row page. expect(reads.scanned).toBe(21); }); + + test('filtered continuation pages charge only their own walk', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const buildQuery = () => + ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => + q.or(q.eq(q.field('name'), 'u10'), q.eq(q.field('name'), 'u20')) + ); + const first = await buildQuery().paginate({ cursor: null, numItems: 1 }); + const second = await buildQuery().paginate({ + cursor: first.continueCursor, + numItems: 1, + }); + + expect(first.page.map((row) => row.name)).toEqual(['u10']); + expect(second.page.map((row) => row.name)).toEqual(['u20']); + expect(reads.documents).toBe(2); + expect(reads.scanned).toBe(21); + }); + + test('a scan-limited page charges through its continuation cursor', async () => { + const rows = Array.from({ length: 20 }, (_, index) => ({ + _id: `id-${index + 1}`, + })); + const filteredPlan = { + source: { type: 'FullTableScan', tableName: 'users' }, + operators: [{ filter: { $eq: ['never', 'matches'] } }], + }; + + class FakeQuery { + state: { query: typeof filteredPlan; type: 'preparing' }; + + constructor(query = filteredPlan) { + this.state = { type: 'preparing', query }; + } + + async paginate(options: { cursor: string | null }) { + if (this.state.query.operators.length > 0) { + return { + continueCursor: 'cursor-10', + isDone: false, + page: [rows[0]], + }; + } + + const offset = Number(options.cursor?.split('-')[1] ?? 0); + const next = offset + 1; + return { + continueCursor: `cursor-${next}`, + isDone: next === rows.length, + page: [rows[next - 1]], + }; + } + } + + const fakeCtx = { + db: { + get: async () => null, + query: () => new FakeQuery(), + }, + }; + const reads = countDocumentReads(fakeCtx as never); + await fakeCtx.db.query().paginate({ cursor: null }); + + expect(reads.documents).toBe(1); + expect(reads.scanned).toBe(10); + }); }); describe('countDocumentReads: paths with nothing hidden', () => { diff --git a/convex/setup.testing.ts b/convex/setup.testing.ts index e889a2071..ecf096068 100644 --- a/convex/setup.testing.ts +++ b/convex/setup.testing.ts @@ -208,6 +208,57 @@ const scanSequence = async (target: any, plan: QueryPlan): Promise => { return rows; }; +const scanPageRange = async ( + target: any, + plan: QueryPlan, + cursor: string | null, + endCursor: string | null +) => { + const QueryConstructor = target?.constructor; + if (typeof QueryConstructor !== 'function') { + throw new Error( + 'countDocumentReads: cannot replay a Convex query without its class.' + ); + } + + let scanned = 0; + let nextCursor = cursor; + if (nextCursor === endCursor) { + return scanned; + } + while (true) { + const replay = new QueryConstructor({ + source: { ...plan.source }, + operators: [], + }); + const page = await replay.paginate({ + cursor: nextCursor, + numItems: 1, + }); + scanned += page.page.length; + if (page.continueCursor === endCursor) { + return scanned; + } + if (page.isDone) { + if (endCursor === null) { + return scanned; + } + throw new Error( + 'countDocumentReads: scan replay passed its pagination boundary.' + ); + } + if ( + typeof page.continueCursor !== 'string' || + page.continueCursor === nextCursor + ) { + throw new Error( + 'countDocumentReads: scan replay did not advance its pagination cursor.' + ); + } + nextCursor = page.continueCursor; + } +}; + const positionOfId = (sequence: any[], id: unknown, from = 0) => { if (id === undefined || id === null) { return -1; @@ -303,19 +354,20 @@ export function countDocumentReads(ctx: { return; } - const sequence = await scanSequence(target, plan as QueryPlan); - // A cursor names the last row of the previous page, so this page starts - // reading right after it. - const start = positionOfId(sequence, options?.cursor ?? null) + 1; - const remaining = Math.max(0, sequence.length - start); - - if (result?.isDone || rows.length === 0) { - reads.scanned += remaining; - return; + const cursor = options?.cursor ?? null; + const endCursor = result?.continueCursor ?? null; + if (!result?.isDone && typeof endCursor !== 'string') { + throw new Error( + 'countDocumentReads: filtered pagination stopped without a ' + + 'continuation cursor.' + ); } - - const last = positionOfId(sequence, rows.at(-1)?._id, start); - reads.scanned += last === -1 ? remaining : last + 1 - start; + reads.scanned += await scanPageRange( + target, + plan as QueryPlan, + cursor, + endCursor + ); }; const wrapIterator = (target: any) => { diff --git a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md index 1bd6a2fc4..a2cdbb182 100644 --- a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md +++ b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md @@ -336,7 +336,8 @@ Review fixes: Error attempts: | Error / failed attempt | Count | Next different move | Resolution | |------------------------|-------|---------------------|------------| -| None yet | 0 | | | +| Filtered continuation cursor treated as a document ID | 1 | Replay the source between the actual opaque cursor boundaries | Fixed and pinned | +| Last returned row used as the scan-limited page boundary | 1 | Stop replay at `continueCursor`, not the last match | Fixed and pinned | Verification evidence: - cwd for all commands: `/Users/mikey/conductor/workspaces/kitcn/moab`. @@ -365,6 +366,19 @@ Verification evidence: port. Unreachable from this diff: the commit touches zero files under `packages/`, and `tooling/scenarios.ts` never references `setup.testing` or `convex/orm`. Closed by CI: PR #399 CI passed in 6m21s, covering that lane. +- Autoclosure P1 review found that filtered pagination treated an opaque + continuation cursor as a document ID and could miss rejected rows on + scan-limited pages. The repair replays the unfiltered source one cursor step + at a time between the real input and output cursor boundaries. +- Autoclosure focused replay -> 16 passed, no type errors, including filtered + continuation and scan-limited cursor boundaries. +- Repair-only P1 autoreview -> clean, patch correct at 0.90 confidence. +- Deslop delta -> 171 findings before and after; no net occurrence increase. +- Fresh scaffold parity exposed `lucide-react` drift from `^1.33.0` to + `^1.34.0`; the repository-owned fixture sync updated the six generated + Next, Start, and Vite snapshots, and `fixtures:check` passed. +- Autoclosure final `bun lint:fix && bun --cwd packages/kitcn build && bun + check` replay exited 0, including fixture parity and every runtime scenario. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -375,6 +389,8 @@ Source-listed case matrix: | Stream paths are already fine | `filterWith` pulls every scanned row | "a JavaScript stream filter is already fully visible" | 300 | `scanned == documents == 300` | passes | done | | `M4: Read bounds` overstates coverage | bucket is 3 rows, would pass under a scan | `where-filtering.test.ts` M4 block | `documents <= 6` at N=60 | `scanned <= 6` at N=300 | passes | done | | Existing invariants must not regress | โ€” | 7 suites, 30 assertions | 174 pass | 174 pass on `scanned` | passes | done | +| Filtered continuation pages | cursor is opaque, not a document ID | two one-row pages | second page can restart at row zero | each page charges only its cursor range | 16-test focused suite | done | +| Scan-limited filtered page | cursor may advance beyond the last match | fake cursor boundary at row 10 | charges 1 returned row | charges all 10 scanned rows | 16-test focused suite | done | Final handoff contract: - Commit line: `feb68240 test(orm): count documents scanned behind .filter()`. @@ -400,7 +416,8 @@ Final handoff contract: `where-clause-compiler.test.ts`, and renaming `documents` would churn 30 call sites for no additional proof. - Verified: see Verification evidence. -- PR body verified: N/A โ€” no PR. +- PR body verified: PR #399 names this plan and the plan exists at its exact + head. Task-style PR body contract: - Preserve any existing `` block. If a changeset is diff --git a/fixtures/next-auth/package.json b/fixtures/next-auth/package.json index be1ed4540..73e06493e 100644 --- a/fixtures/next-auth/package.json +++ b/fixtures/next-auth/package.json @@ -9,7 +9,7 @@ "convex": "1.44.0", "hono": "4.12.9", "kitcn": "workspace:*", - "lucide-react": "^1.33.0", + "lucide-react": "^1.34.0", "next": "16.2.6", "next-themes": "^0.4.6", "react": "19.2.4", diff --git a/fixtures/next/package.json b/fixtures/next/package.json index 831ea8450..5a809c6c0 100644 --- a/fixtures/next/package.json +++ b/fixtures/next/package.json @@ -8,7 +8,7 @@ "convex": "1.44.0", "hono": "4.12.9", "kitcn": "workspace:*", - "lucide-react": "^1.33.0", + "lucide-react": "^1.34.0", "next": "16.2.6", "next-themes": "^0.4.6", "react": "19.2.4", diff --git a/fixtures/start-auth/package.json b/fixtures/start-auth/package.json index 68cb1473e..4b9152b66 100644 --- a/fixtures/start-auth/package.json +++ b/fixtures/start-auth/package.json @@ -17,7 +17,7 @@ "convex": "1.44.0", "hono": "4.12.9", "kitcn": "workspace:*", - "lucide-react": "^1.33.0", + "lucide-react": "^1.34.0", "react": "^19.2.6", "react-dom": "^19.2.6", "shadcn": "latest", diff --git a/fixtures/start/package.json b/fixtures/start/package.json index c88fa9370..ad647861d 100644 --- a/fixtures/start/package.json +++ b/fixtures/start/package.json @@ -16,7 +16,7 @@ "convex": "1.44.0", "hono": "4.12.9", "kitcn": "workspace:*", - "lucide-react": "^1.33.0", + "lucide-react": "^1.34.0", "react": "^19.2.6", "react-dom": "^19.2.6", "shadcn": "latest", diff --git a/fixtures/vite-auth/package.json b/fixtures/vite-auth/package.json index a906d57a5..faaa2aeff 100644 --- a/fixtures/vite-auth/package.json +++ b/fixtures/vite-auth/package.json @@ -11,7 +11,7 @@ "convex": "1.44.0", "hono": "4.12.9", "kitcn": "workspace:*", - "lucide-react": "^1.33.0", + "lucide-react": "^1.34.0", "react": "^19.2.6", "react-dom": "^19.2.6", "shadcn": "latest", diff --git a/fixtures/vite/package.json b/fixtures/vite/package.json index b73a8b5dc..27857d54a 100644 --- a/fixtures/vite/package.json +++ b/fixtures/vite/package.json @@ -10,7 +10,7 @@ "convex": "1.44.0", "hono": "4.12.9", "kitcn": "workspace:*", - "lucide-react": "^1.33.0", + "lucide-react": "^1.34.0", "react": "^19.2.6", "react-dom": "^19.2.6", "shadcn": "latest", From fee68a9e62b9fd245b2bf472d5badc7640d2e1e0 Mon Sep 17 00:00:00 2001 From: zbeyens Date: Tue, 25 Aug 2026 22:42:59 +0200 Subject: [PATCH 5/5] fix bounded scan replay accounting --- convex/orm/read-counting.test.ts | 57 ++++++++++++++++++- convex/setup.testing.ts | 50 ++++++++++++---- ...an-aware-read-counting-for-filter-paths.md | 12 ++++ 3 files changed, 108 insertions(+), 11 deletions(-) diff --git a/convex/orm/read-counting.test.ts b/convex/orm/read-counting.test.ts index e02f9dad7..cdf30c663 100644 --- a/convex/orm/read-counting.test.ts +++ b/convex/orm/read-counting.test.ts @@ -170,6 +170,59 @@ describe('countDocumentReads: rows rejected by .filter()', () => { expect(reads.scanned).toBe(3); }); + test('a leading limit bounds the source before filtering', async ({ + ctx, + }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const query = ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) as any; + const rows = await query + .limit(5) + .filter((q: any) => q.eq(q.field('name'), 'u2')) + .collect(); + + expect(rows.map((row: any) => row.name)).toEqual(['u2']); + expect(reads.documents).toBe(1); + expect(reads.scanned).toBe(5); + }); + + test('stacked serialized limits fail loudly', async ({ ctx }) => { + await seedBucket(ctx); + + const reads = countDocumentReads(ctx); + const query = ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => + q.or(q.eq(q.field('name'), 'u2'), q.eq(q.field('name'), 'u7')) + ) as any; + await expect(query.limit(10).limit(1).collect()).rejects.toThrow( + 'more than one serialized limit' + ); + expect(reads.documents).toBe(0); + expect(reads.scanned).toBe(0); + }); + + test('an interleaved filter after a limit fails loudly', async ({ ctx }) => { + await seedBucket(ctx); + + countDocumentReads(ctx); + const query = ctx.db + .query('users') + .withIndex('by_status', (q) => q.eq('status', 'bucket')) + .filter((q) => q.eq(q.field('name'), 'u2')) as any; + + await expect( + query + .limit(5) + .filter((q: any) => q.eq(q.field('status'), 'bucket')) + .collect() + ).rejects.toThrow('filter follows a post-filter limit'); + }); + test('a filtered page charges for the walk, not the page', async ({ ctx, }) => { @@ -232,7 +285,9 @@ describe('countDocumentReads: rows rejected by .filter()', () => { } async paginate(options: { cursor: string | null }) { - if (this.state.query.operators.length > 0) { + if ( + this.state.query.operators.some((operator) => 'filter' in operator) + ) { return { continueCursor: 'cursor-10', isDone: false, diff --git a/convex/setup.testing.ts b/convex/setup.testing.ts index ecf096068..452a24f69 100644 --- a/convex/setup.testing.ts +++ b/convex/setup.testing.ts @@ -172,22 +172,52 @@ const planHasFilter = (plan: QueryPlan | null) => plan !== null && plan.operators.some((operator) => 'filter' in operator); const planLimit = (plan: QueryPlan | null) => { - for (const operator of plan?.operators ?? []) { + const limits = (plan?.operators ?? []).flatMap((operator) => + typeof operator.limit === 'number' ? [operator.limit] : [] + ); + if (limits.length > 1) { + throw new Error( + 'countDocumentReads: cannot replay more than one serialized limit.' + ); + } + return limits[0] ?? Number.POSITIVE_INFINITY; +}; + +const scanReplayOperators = (plan: QueryPlan) => { + const leadingLimits: { limit: number }[] = []; + let sawFilter = false; + let sawPostFilterLimit = false; + + for (const operator of plan.operators) { + if ('filter' in operator) { + if (sawPostFilterLimit) { + throw new Error( + 'countDocumentReads: cannot replay a query whose filter follows ' + + 'a post-filter limit.' + ); + } + sawFilter = true; + continue; + } if (typeof operator.limit === 'number') { - return operator.limit; + if (sawFilter) { + sawPostFilterLimit = true; + } else { + leadingLimits.push({ limit: operator.limit }); + } } } - return Number.POSITIVE_INFINITY; + + return leadingLimits; }; /** - * Replay `plan`'s source with every operator dropped. + * Replay `plan`'s source without filters or post-filter limits. * * The result is the sequence of documents Convex walks to answer the real - * query: `.filter()` and `.limit()` decide which of them survive, never which - * of them are read. Running it through the engine rather than re-evaluating the - * predicate here keeps the two passes in the same order, so the filtered result - * is always a subsequence of this one. + * Leading limits bound the source before filtering and therefore stay in the + * replay. Running the source through the engine keeps the filtered result a + * subsequence of this one. */ const scanSequence = async (target: any, plan: QueryPlan): Promise => { const QueryConstructor = target?.constructor; @@ -199,7 +229,7 @@ const scanSequence = async (target: any, plan: QueryPlan): Promise => { const replay = new QueryConstructor({ source: { ...plan.source }, - operators: [], + operators: scanReplayOperators(plan), }); const rows: any[] = []; for await (const row of replay) { @@ -229,7 +259,7 @@ const scanPageRange = async ( while (true) { const replay = new QueryConstructor({ source: { ...plan.source }, - operators: [], + operators: scanReplayOperators(plan), }); const page = await replay.paginate({ cursor: nextCursor, diff --git a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md index a2cdbb182..109a15d67 100644 --- a/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md +++ b/docs/plans/2026-08-21-384-scan-aware-read-counting-for-filter-paths.md @@ -338,6 +338,7 @@ Error attempts: |------------------------|-------|---------------------|------------| | Filtered continuation cursor treated as a document ID | 1 | Replay the source between the actual opaque cursor boundaries | Fixed and pinned | | Last returned row used as the scan-limited page boundary | 1 | Stop replay at `continueCursor`, not the last match | Fixed and pinned | +| Replay dropped a source-bounding limit before a filter | 1 | Preserve leading limits and reject uncountable interleavings | Fixed and pinned | Verification evidence: - cwd for all commands: `/Users/mikey/conductor/workspaces/kitcn/moab`. @@ -379,6 +380,15 @@ Verification evidence: Next, Start, and Vite snapshots, and `fixtures:check` passed. - Autoclosure final `bun lint:fix && bun --cwd packages/kitcn build && bun check` replay exited 0, including fixture parity and every runtime scenario. +- Exact-head review found that dropping a limit before a filter could inflate a + bounded scan. The second repair preserves leading source limits, rejects + filter-limit-filter and stacked-limit plans the simulator cannot count + honestly, and raises the focused suite to 19 passing tests. +- Second repair-only P1 autoreview -> clean, patch correct at 0.87 confidence. +- Final deslop delta -> 171 findings before and after, with zero occurrence + changes and no score change. +- Post-limit-repair `bun lint:fix && bun --cwd packages/kitcn build && bun + check` replay exited 0, including fixture parity and every runtime scenario. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -391,6 +401,8 @@ Source-listed case matrix: | Existing invariants must not regress | โ€” | 7 suites, 30 assertions | 174 pass | 174 pass on `scanned` | passes | done | | Filtered continuation pages | cursor is opaque, not a document ID | two one-row pages | second page can restart at row zero | each page charges only its cursor range | 16-test focused suite | done | | Scan-limited filtered page | cursor may advance beyond the last match | fake cursor boundary at row 10 | charges 1 returned row | charges all 10 scanned rows | 16-test focused suite | done | +| Leading source limit | limit executes before filtering | `limit(5).filter(...)` | replay drains the full range | replay stops after 5 scanned rows | 19-test focused suite | done | +| Uncountable operator interleaving | simulator does not preserve full operator order | filter-limit-filter and stacked limits | counter can claim a false bound | counter throws instead of lying | 19-test focused suite | done | Final handoff contract: - Commit line: `feb68240 test(orm): count documents scanned behind .filter()`.