Skip to content

fix(storage): retry contended message writes and preserve cancellation - #287

Merged
hetaoBackend merged 3 commits into
mainfrom
fix/sqlite-message-write-contention
Sep 21, 2026
Merged

hetaoBackend merged 3 commits into
mainfrom
fix/sqlite-message-write-contention

Conversation

@hetaoBackend

@hetaoBackend hetaoBackend commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Change

A foreign SQLite writer that outlasts the native five-second busy timeout can fail v2 message projection, poison the ordered event lane, and terminate an active turn. This change retries message-upsert transaction admission with short native waits and asynchronous backoff within a ten-second budget. The mutation callback is never replayed, and the shared connection timeout is restored before callbacks and asynchronous waits.

Cancellation carries the exact turn lease signal separately from semantic event snapshots. Cancellation stops ordinary display-message lock waits while preserving normal aborted settlement and history reconciliation. An immediately available cleanup write can still commit after cancellation. Complete messages carrying tool calls preserve executed tool facts even during abort cleanup and retain the bounded write-lock retry policy. Unrelated errors and failures after mutation starts still fail closed. The scope is the reproduced v2 message-upsert path; other SQLite writers retain their existing policy.

Related to #282.

Validation

  • PASS: pnpm verify, full profile, all 14 applicable gates on clean commit d44d687d2e8be34f778f6282f9ca9c2c6f7cf466 (macOS arm64, Node v26.4.0). Capability coverage: 157 files / 4,217 tests.
  • PASS: twelve focused real-SQLite repository/event-pipeline tests covering contention recovery, bounded waits, connection timeout restoration, cancellation, tool completion persistence after cancellation before/during contention, callback rollback without replay, and preservation of unrelated failures.
  • PASS: full CLI with a loopback synthetic provider and an independent SQLite writer holding the lock for 6.5 seconds. The reproduced exit-4 failure now exits 0 with a completed turn, idle session, and canonical user/assistant history.
  • PASS: full CLI SIGINT regression with a 1.5-second foreign lock: exit 130, durable turn/session/terminal all aborted, no session error, and cancelled assistant output removed from canonical history. This checks durable state in addition to the exit code.
  • PASS: full CLI SIGINT during an actual Bash command, without a foreign writer. Reopened display history retains the tool call/result, canonical history retains user/assistant/toolResult, and turn/session/terminal stay aborted. The same regression fails on the previous PR head and passes on the original merge base.
  • Performance: perf:full required. The previous full run was INCONCLUSIVE because of excessive sample spread; this is neither a pass nor a confirmed regression. A passing run for the latest PR head and intended base is pending CI and must be linked before merge.
  • NOT RUN: Linux/Windows execution, real-provider acceptance, npm release-archive installation, and remote performance validation. The POSIX SIGINT CLI regression explicitly skips Windows; repository/pipeline tests remain in the cross-platform capability suite. Deliberate foreign-writer contention does not establish acceptance for the reporter's full multi-session workload.

Publication and contribution checks

  • I have permission to contribute these changes under the existing licenses applicable to the changed files/packages; no external material was imported and existing notices are preserved.
  • No credentials, account data, real user content, internal source history or private review material is included.
  • Added source files were reviewed before regenerating release/public-source.json; the new Vitest file is registered in test/vitest-suites.json. The CLI regression uses the existing BYOK gate.
  • No shared English/Chinese documentation changes are needed; no configuration or user-facing interface was added. Offline fixtures are not described as live-service acceptance.

Maintainer handoff

Publication scope or license changes: none beyond the reviewed retry helper and regression test recorded in the source inventory; existing licenses apply.

Shared-source port: pending.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@hetaoBackend hetaoBackend added bug Something isn't working cli Standalone mcode: TUI, headless, ACP and source builds/tooling perf:full Run the full performance suite; required for performance-sensitive changes labels Sep 21, 2026
@1anZhang

Copy link
Copy Markdown
Collaborator

[P2] Preserve tool completion messages after cancellation

Passing the lease signal to every display-message upsert in agent-projection.ts:94 also cancels necessary tool completion messages emitted after Ctrl+C. runWithWriteLock rejects an already-aborted signal before attempting the transaction, and TurnCommitPipeline swallows that rejection. This happens even without SQLite contention: the tool call and result remain in canonical history, but their display message is never persisted and is missing when the conversation is reopened.

I reproduced this with the built CLI and a loopback synthetic provider: start a Bash command that creates a marker and then sleeps, send SIGINT after the marker appears, and inspect the persisted display rows and canonical history after the CLI exits.

Revision Persisted display roles Canonical history roles
Merge base 29addf7b user, assistant user, assistant, toolResult
PR head d3d6c687 user user, assistant, toolResult

Both runs exited with code 130 and persisted an aborted turn. The same assertion that the tool message remains in display history passes on the merge base and fails on the PR head (macOS arm64, Node 22.22.2).

Please preserve required post-cancellation tool completion projections while allowing blocked writes to stop waiting, and cover ordinary Ctrl+C during tool execution with a regression test that does not involve a foreign writer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cli Standalone mcode: TUI, headless, ACP and source builds/tooling perf:full Run the full performance suite; required for performance-sensitive changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants