test(sessions): characterize record IDs before immutable history - #6499
test(sessions): characterize record IDs before immutable history#6499mmabrouk wants to merge 3 commits into
Conversation
Two comments claimed session records arrive ordered by a uuid7 `id`. No uuid7 is minted on this path: `record_id` is a uuid5 for tool-family records and a uuid4 otherwise, and neither is time ordered. Two more comments named an older ordering that has since changed. The real order is `(timestamp, created_at, record_index)`, set in api/oss/src/dbs/postgres/sessions/records/dao.py:157-161. Behavior is unchanged; these are comment-only edits. Found by the stable record-ID spike (work package D). Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Characterization tests for what a repeated `record_id` means today, and what an immutable insert (ON CONFLICT DO NOTHING) would change. They pass on current behavior. Each test carries a comment naming what must change when inserts become immutable. Runner (9 tests): a store simulator applies either the current upsert rule or an insert-only rule to the POST bodies the emitter actually sends, then feeds the resulting rows to the real reconstructMessages, so the loss is asserted rather than argued. Covers three argument snapshots then a result, interleaved tool calls that re-open a flushed id, the idle-timer flush, a tool_result sent twice, an exact interaction retry, a resume re-emission across turns, a duplicate terminal event, and the drop-counter defect. API (8 tests): the real _dedupe_values, _UPSERT_UPDATED_COLUMNS, map_record_event_to_dbe and build_wire_messages. Pins the six overwritten columns, the arguments-repaired-but-row-re-sorted behavior, and the wire-message reconstruction that loses tool arguments under an immutable insert. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
The gate records-invariants.md requires before immutable-history work. Inventories every durable record producer, classifies every repeated record_id as an exact retry, a progressive update, or a resume re-emission, and reports what an immutable insert would break with the concrete case. Finding: one class breaks, and it is a producer bug. A tool call is written before its arguments arrive, and a later snapshot repairs the row. Under DO NOTHING the durable call keeps empty arguments. Both paths that cause it live in the runner's one-open-tool-slot design. Fixing the producer also removes a live transcript ordering defect, because the upsert overwrites the primary read-order key. Also confirms two defects: the turn-end drain consumes the drop counter one line before the run handler reads it, so noteRecordsIncomplete is unreachable; and four comments named an ordering that never existed. Reports two more found on the way: the records worker acknowledges Redis messages it never wrote, and Enterprise record retention raises on a column that does not exist. Recommends Option A with the producer fix landing first, and lists the migration risk for old rows. Does not select an option and changes no behavior. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mmabrouk
changed the title
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Review result
Recommendation: hold. I reviewed the complete 7-file delta at exact head 90ff86a1a074c570da3c463438ba476e7a91d57d against its declared base, agent/session-execution-rfc. The characterization tests expose useful evidence, but the new runner test file breaks the runner package's required TypeScript typecheck.
User problem
The broader RFC needs to know whether session records can become immutable without losing conversation history. Today, repeated stable record IDs can mean either an exact retry or a later, more complete tool event. Treating both as insert-only would preserve retries but can retain empty tool arguments or empty results.
How the change works
This PR does not change runtime behavior. It:
- corrects four comments about record ordering;
- adds runner tests that capture emitted request bodies, simulate current upsert and proposed insert-only storage, and reconstruct messages from both;
- adds API tests around the real batch deduplication and wire-message reconstruction helpers;
- documents the repeated-ID classes, related loss paths, and migration questions.
Issue coverage
No GitHub issue is directly linked from the PR body or timeline. The parent requirements document cites #5496 and #5594 for silent record loss. This PR partially supports those issues with characterization and analysis, but it fixes neither issue because it changes no behavior. In particular, #5496's API publish, worker acknowledgement, pending-entry reclaim, and stable identity paths remain unchanged. The batch deduplication behavior behind #5594 already exists in the declared base and is only characterized here.
Dependencies and overlap
This is not independent. It is stacked on agent/session-execution-rfc, represented by PR #6495, and should be reviewed as that branch's delta. It also identifies two changes that overlap PR #6502: acknowledging Redis records only after a Postgres commit, and repairing the enterprise retention query. Those findings should not be implemented twice.
The proposed immutable-insert work also depends on a producer change that prevents partial tool calls and placeholder results from reusing an ID before their final payload exists. This PR documents that dependency but does not implement it.
Tests and missing environments
Local results on the exact head:
- API characterization test: 8 passed.
- Runner characterization test: 9 passed.
- Runner
pnpm run typecheck: failed with six errors in the new test file. git diff --check: passed.
At this SHA, GitHub skipped the API, services, runner, runner integration, runner acceptance, web, mobile, image-build, and Helm checks. Formatting, lint, secret scanning, contribution checks, and the documentation preview passed.
The API tests use helper-level simulation rather than live Postgres. The runner tests mock HTTP ingestion and simulate storage in memory. No live Redis worker, Postgres conflict behavior, multi-process retry, enterprise retention, browser replay, mobile replay, or remote harness environment was exercised.
Review summary
- Risky:
services/runner/tests/unit/record-id-semantics.test.ts:269- the added characterization suite fails the package's required TypeScript typecheck. - Missing tests: live Postgres and Redis worker behavior, retry and reclaim paths, enterprise retention, browser and mobile replay, and real harness event sequences.
After the typecheck failure is fixed, review this as a stacked, evidence-only spike against PR #6495. Do not treat it as proof that the proposed producer or storage changes work in production.
| // Harness reconstruction inherits the loss: the rebuilt conversation tells the model | ||
| // the agent called bash with no arguments. | ||
| const rebuilt = reconstructMessages(insertOnly); | ||
| const blocks = rebuilt[0].content as Array<Record<string, unknown>>; |
There was a problem hiding this comment.
Must fix: this added test passes at runtime but breaks the runner package's required pnpm run typecheck. At this line, TypeScript rejects the direct cast from string | ContentBlock[] to Record<string, unknown>[]. The same error recurs at lines 338, 347, and 416, and the query-suffixed imports at lines 286 and 540 cannot be resolved by TypeScript. A clean checkout reports six errors and exits 2, so this PR cannot pass the package's documented verification command. Narrow content with Array.isArray (or keep the declared ContentBlock[] type), and replace or type the cache-busting import strategy so tsc --noEmit resolves it.
1 participant
Session records currently use the same ID for both exact delivery retries and progressive tool updates. Changing the table to immutable inserts without understanding those cases can lose tool arguments or output.
This change inventories repeated record IDs and adds characterization tests for the runner and Postgres data-access layer. It finds that exact retries already fit immutable inserts, while progressive tool calls and tool results need a producer change first. It also finds that terminal records need stable producer IDs.
Issue coverage
This supports #5496 and #5594 but does not fix their runtime failures. The acknowledgement fix is in #6502.
Dependencies and limits
Independent tests and documentation on the RFC branch. Before changing storage, confirm the inventory includes every intentional progressive update.
How to review