Skip to content

fix: make sqlite session entry writes atomic#91491

Closed
jalehman wants to merge 1 commit into
mainfrom
fix/sqlite-session-entry-atomic
Closed

fix: make sqlite session entry writes atomic#91491
jalehman wants to merge 1 commit into
mainfrom
fix/sqlite-session-entry-atomic

Conversation

@jalehman

@jalehman jalehman commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR hardens the SQLite session metadata path that landed in #91322 by moving ordinary hot session-entry writes off the stale whole-store replacement path.

  • Adds an atomic SQLite single-entry patch path that re-reads the current session_entries row inside the per-agent SQLite write transaction, merges the caller's explicit patch, and upserts only that row.
  • Threads explicit patch intent through the existing session-store facade so normal metadata touches can preserve unrelated concurrent fields written by another process.
  • Refactors safe production adapters to pass narrow patches for agent run metadata, slash-command session updates, auto-reply session updates, and gateway admission touches.
  • Leaves the whole-store replacement path for maintenance/migration/pruning/capping/legacy-key cleanup and other multi-row operations; this PR reduces exposure to that path, but does not try to make whole-store replacement generally safe.
  • Intentionally does not introduce the broader typed session/transcript schema. This is the tactical post-refactor: move session metadata to SQLite #91322 safety slice on top of the current cache_entries blob backend.

Review focus: the row-patch semantics, the cache reconciliation after a row patch, and whether each migrated caller provides a patch that accurately represents its intended write.

Linked context

Related #91322, which moved session metadata from sessions.json into SQLite but retained the old load/mutate/delete-and-reinsert whole-store write shape for normal metadata updates.

Related tracker #88838 for the broader core session/transcript runtime-state SQLite migration.

Supersession notes for Josh's existing stack:

Requested by Josh during review of the post-#91322 SQLite session-store behavior.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: ordinary session metadata writes could load a whole-store snapshot, run a callback, then replace all SQLite session_entries rows, which can clobber unrelated fields written by another process between the callback read and the final transaction.
  • Real environment tested: local OpenClaw Codex worktree using the repo's node scripts/run-vitest.mjs wrapper for focused session/agent tests.
  • Exact steps or command run after this patch: node scripts/run-vitest.mjs src/config/sessions.cache.test.ts src/config/sessions.test.ts src/agents/command/session-store.test.ts src/agents/command/attempt-execution.shared.test.ts
  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output): command passed 2 Vitest shards, 4 test files, 129 tests in 16.78s.
  • Observed result after fix: added regression coverage proves updateSessionStoreEntry preserves an external SQLite write made while its update callback is running, while still applying the local patch.
  • What was not tested: full suite, broad Crabbox/Testbox gate, and live gateway session workflow.
  • Proof limitations or environment constraints: this is focused local regression proof for a narrow storage race; broad release verification should still run before merge.
  • Before evidence (optional but encouraged): prior refactor: move session metadata to SQLite #91322 behavior used replaceSqliteSessionStore, which deletes all rows for the session_entries scope and reinserts from the callback's previously loaded store snapshot.

Tests and validation

Commands run:

  • git diff --check
  • node scripts/run-vitest.mjs src/config/sessions.cache.test.ts src/config/sessions.test.ts src/agents/command/session-store.test.ts src/agents/command/attempt-execution.shared.test.ts
  • .agents/skills/autoreview/scripts/autoreview --mode local

Regression coverage added:

  • src/config/sessions.test.ts now covers preserving an external SQLite write while an updateSessionStoreEntry callback is in flight.
  • src/agents/command/session-store.test.ts now verifies after-run persistence passes a narrow metadata patch rather than a broad entry-shaped persistence payload.

Autoreview result: clean, no accepted/actionable findings.

Risk checklist

Did user-visible behavior change? No intended user-facing behavior change; session metadata persistence should be more concurrency-safe.

Did config, environment, or migration behavior change? No.

Did security, auth, secrets, network, or tool execution behavior change? No.

Highest-risk area: incorrectly classifying a caller as safe for row patching when its callback actually depends on deletion, conditional persistence, legacy key cleanup, maintenance, or multi-row invariants.

Mitigation: this PR only uses the row path when callers provide explicit patches. attempt-execution.shared.ts opts out when clearedFields or shouldPersist are present, because those require semantics beyond a simple merge patch. Whole-store replacement remains available for maintenance and structurally unsafe operations.

Current review state

Draft PR for review. Next action is maintainer review of the row-patch contract and the supersession relationship to the older session-store PR stack.

Still waiting on broad CI/Testbox proof before this should be treated as land-ready.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels Jun 8, 2026
@clawsweeper

clawsweeper Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 8, 2026, 3:22 PM ET / 19:22 UTC.

Summary
The PR adds a SQLite single-session-entry patch path and threads explicit metadata patches through selected agent, gateway, and auto-reply session writes.

PR surface: Source +153, Tests +39. Total +192 across 9 files.

Reproducibility: yes. from source inspection, though I did not run tests in this read-only review. Current main writes the whole SQLite session_entries scope from the writer snapshot, and the PR's new test simulates an external SQLite write during a callback.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Fix the reconciled-row return/in-memory sessionStore path and add a focused regression for it.
  • [P1] Add real behavior proof from an after-fix OpenClaw session/gateway run or obtain a maintainer proof override, redacting private details.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports focused local Vitest proof only; external PR merge still needs real behavior proof or a maintainer proof override. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The new row patch can preserve the SQLite row while returning a stale mutator object to migrated callers, leaving in-memory session state behind the persisted row and risking a later clobber from the same process.
  • [P1] The PR body reports focused Vitest proof only; there is no real gateway/session workflow proof or maintainer override yet for this external draft.
  • [P1] The PR is intentionally labeled maintainer and still asks for review of the row-patch contract, so it should not be treated as land-ready cleanup.

Maintainer options:

  1. Fix Returned Session State Before Merge (recommended)
    Update the single-entry persistence path so direct updateSessionStore callers receive the reconciled row after the SQLite transaction, then cover a migrated caller with an external-write regression.
  2. Hold For Maintainer Contract Review
    Keep the draft open until maintainers confirm which session write callers are safe for row patching and which must remain on whole-store replacement.
  3. Accept Narrow Cache-Only Semantics
    Maintainers could intentionally accept that only the SQLite row/cache is reconciled, but that should be explicit because migrated callers currently treat the return value as persisted state.

Next step before merge

  • [P1] The PR needs maintainer handling because it is a protected draft with missing real behavior proof, plus a narrow correctness fix before it can be merge-ready.

Security
Cleared: The diff is limited to session-state persistence code and focused tests, with no dependency, workflow, secret, package, or supply-chain surface changes found.

Review findings

  • [P2] Return the reconciled row after a SQLite patch — src/config/sessions/store.ts:536
Review details

Best possible solution:

Keep the atomic row-patch approach, but make direct updateSessionStore callers receive the reconciled persisted row, add regression coverage for that returned/in-memory state, and require maintainer review plus real behavior proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection, though I did not run tests in this read-only review. Current main writes the whole SQLite session_entries scope from the writer snapshot, and the PR's new test simulates an external SQLite write during a callback.

Is this the best way to solve the issue?

No, not yet. The row-patch direction is the right layer for this tactical safety slice, but the implementation still returns stale mutator results to migrated callers instead of the reconciled persisted row.

Full review comments:

  • [P2] Return the reconciled row after a SQLite patch — src/config/sessions/store.ts:536
    The new single-entry path can re-read and persist a row that includes concurrent fields, but updateSessionStore still returns the mutator's pre-reconciliation object. The migrated callers use that return value to refresh their in-memory sessionStore, so after an external SQLite write they can keep a stale entry and later write from it. Have the patch path return or otherwise expose the reconciled persisted row to direct callers, and cover that return/in-memory state in a regression.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 9220761fbaea.

Label changes

Label changes:

  • add P2: This is a normal-priority session-state correctness hardening PR with limited blast radius but a blocking implementation issue.
  • add merge-risk: 🚨 session-state: The diff changes how session metadata writes reconcile concurrent SQLite row state, and an incorrect merge can stale or later clobber session metadata.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused local Vitest proof only; external PR merge still needs real behavior proof or a maintainer proof override. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a normal-priority session-state correctness hardening PR with limited blast radius but a blocking implementation issue.
  • merge-risk: 🚨 session-state: The diff changes how session metadata writes reconcile concurrent SQLite row state, and an incorrect merge can stale or later clobber session metadata.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused local Vitest proof only; external PR merge still needs real behavior proof or a maintainer proof override. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +153, Tests +39. Total +192 across 9 files.

View PR surface stats
Area Files Added Removed Net
Source 7 170 17 +153
Tests 2 40 1 +39
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 9 210 18 +192

What I checked:

  • Current main behavior: Current main still saves normal session-store updates by calling replaceSqliteSessionStore after the mutator, so a stale writer snapshot can replace the full session_entries scope. (src/config/sessions/store.ts:463, 9220761fbaea)
  • Current SQLite replacement path: The existing SQLite save path deletes all rows in the session_entries scope and reinserts the writer's store snapshot. (src/config/sessions/store-sqlite.ts:150, 538d36eaaaa6)
  • PR patch path: The PR adds patchSqliteSessionEntry, which re-reads one current row inside the agent DB write transaction, merges the explicit patch, and upserts that row. (src/config/sessions/store-sqlite.ts:185, af712b51fb8a)
  • Blocking review finding: After saveSessionStoreUnlocked reconciles the patched row into the writer store/cache, updateSessionStore still returns the original mutator result; migrated callers then refresh their in-memory sessionStore with that stale object. (src/config/sessions/store.ts:536, af712b51fb8a)
  • Migrated caller using stale return: updateSessionStoreAfterAgentRun uses the updateSessionStore return value as persisted sessionStore state after adding single-entry persistence, so it can keep a stale in-memory entry after a concurrent row write. (src/agents/command/session-store.ts:321, af712b51fb8a)
  • Focused regression coverage: The new test proves the SQLite row on disk preserves an external write while updateSessionStoreEntry's callback is in flight, but it does not assert the direct updateSessionStore return or caller in-memory store is reconciled. (src/config/sessions.test.ts:1051, af712b51fb8a)

Likely related people:

  • Peter Steinberger: Commit 538d36e moved session metadata to SQLite and blame shows the replaceSqliteSessionStore delete-and-reinsert path in store-sqlite.ts. (role: introduced current SQLite session metadata behavior; confidence: high; commits: 538d36eaaaa6, 224c24006a07; files: src/config/sessions/store.ts, src/config/sessions/store-sqlite.ts)
  • Josh Lehman: History shows prior merged session-store cache work by Josh, and this PR changes the same session-store atomicity surface rather than appearing as an unrelated one-off proposal. (role: session-store adjacent contributor and proposed patch author; confidence: medium; commits: 175c77017115, 1212328c8da1, 78a7ac98265f; files: src/config/sessions/store.ts, src/config/sessions.test.ts, src/agents/command/session-store.ts)
  • Shakker: Local history and blame show recent current-main touches to session-store files, including the snapshot commit carrying the current session-store sources in this checkout. (role: recent adjacent area contributor; confidence: medium; commits: a04de1a0ceac, c2e93c76bd43; files: src/config/sessions/store.ts, src/config/sessions/store-sqlite.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 8, 2026
@jalehman jalehman closed this Jun 10, 2026
@jalehman jalehman mentioned this pull request Jun 10, 2026
Closed
@jalehman

jalehman commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by the #88838 stack, per maintainer decision.

Context: #91322 was reverted on main on 2026-06-10 (7f1d82ab25, "defer session metadata sqlite"), so the cache_entries-backed session store this PR hardens no longer exists on main and the branch now conflicts.

The substance of this PR survives in the stack that will reland SQLite sessions:

Tracker: #88838.

@jalehman jalehman mentioned this pull request Jun 11, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 16, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jalehman