Skip to content

Autosave must yield to the user: make the shared mutation guard ownership-aware#138

Merged
dovvnloading merged 1 commit into
mainfrom
qt-removal/r6-autosave-yields-to-user
Jul 26, 2026
Merged

Autosave must yield to the user: make the shared mutation guard ownership-aware#138
dovvnloading merged 1 commit into
mainfrom
qt-removal/r6-autosave-yields-to-user

Conversation

@dovvnloading

Copy link
Copy Markdown
Owner

The last remaining confirmed finding from the R6.6/R6.7/R6.8 audit — deliberately deferred out of #137 rather than bolted onto it.

The bug

chat_library.py's _mutation_in_progress flag serializes loadChat/saveChat/newChat by dropping a blocked intent and warning "Another chat operation is already in progress."

That contract is honest when only user-initiated intents can hold the flag — the user really did start two things. R6.6 then had a background autosave task claim the same flag, and the asymmetry went unnoticed: a tick that happened to be mid-write made the user's own Save/Load/New Chat vanish, with a warning naming an operation they never started.

A background convenience feature must never beat the user to their own data.

The fix

The guard now records who holds it, and the two directions differ:

arriving holder behavior
user autosave wait the tick out, then proceed (new)
user another user drop + warn, exactly as before
autosave anyone skip this interval, exactly as before

The wait is bounded — AUTOSAVE_YIELD_TIMEOUT_SECONDS = 2.0, roughly 40× the measured 10–50ms tick. A tick genuinely stuck on sqlite's own 30s lock timeout degrades to the pre-fix drop rather than freezing the UI, but with a message that names autosave instead of "another chat operation" — which is what made the original warning read as a bug. Every outcome is at least as good as before; the common one is strictly better, in that the Save just works.

The guard's shape now lives in one factory, _new_mutation_guard(), so chat_library, autosave and the tests can't drift apart on it. Its release Event is constructed with no running loop on purpose — safe since 3.10, and register_chat_library really is called outside a loop, which is the exact hazard that broke R6.6's own create_task call.

Mutation testing caught two bad tests before this shipped

The first attempt failed its own bar, twice:

  1. The test used a hand-written double that reimplemented the claim/release itself. Mutating the real _guarded_tick — dropping its owner tag, or its release signal — left the test green. The production wiring was never under test at all. Rewritten to drive the real register_autosave loop with a deliberately slow save, bounded by asyncio.wait_for so a lost wakeup fails rather than merely running slow.

  2. That exposed a second gap: the rewritten test only ever observed the first tick to claim the guard, so removing _guarded_tick's released.clear() still survived. But without it, every tick after the first wakes a waiter on a stale signal, which re-checks, finds the guard still held, and drops the user's intent exactly as before. A fix that works once and then quietly stops working is worse than no fix — so there's now a test pinning the steady state, not just the first run.

All 5 mutants killed, including both that initially survived.

Verification

2375 pytest (+5), 1053 vitest unchanged (no frontend touched), tsc/eslint/build clean, Qt burn-down gate unchanged at 152.

Also re-driven against a scratch copy of a real ~/.graphlink/chats.db: 5 real legacy chats still load and tick without a spurious rewrite through the rewritten guard. Real chats.db verified untouched.

With this, every confirmed finding from the audit is closed except one documented as knowingly unfixable safely: the crash sentinel reporting a phantom crash when a second instance is live, which needs a pid-liveness check with no safe portable form (os.kill signal-0 is POSIX-only; on Windows it would terminate the other process).

…ship-aware

The last remaining confirmed finding from the R6.6/R6.7/R6.8 audit, deferred
out of #137 rather than bolted onto it.

chat_library.py's _mutation_in_progress flag serializes loadChat/saveChat/
newChat by DROPPING a blocked intent and warning "Another chat operation is
already in progress." That contract is honest when only user-initiated intents
can hold the flag - the user really did start two things. R6.6 then had a
background autosave task claim the same flag, and the asymmetry went unnoticed:
a tick that happened to be mid-write made the user's own Save/Load/New Chat
vanish, with a warning naming an operation they never started. A background
convenience feature must never beat the user to their own data.

The guard now records WHO holds it, and the two directions differ:
  user arrives, autosave holds     -> wait the tick out, then proceed
  user arrives, another user holds -> drop + warn, exactly as before
  autosave arrives, anyone holds   -> skip this interval, exactly as before

The wait is bounded (AUTOSAVE_YIELD_TIMEOUT_SECONDS = 2.0, ~40x the measured
10-50ms tick). A tick genuinely stuck on sqlite's own 30s lock timeout
degrades to the pre-fix drop rather than freezing the UI - but with a message
that names autosave instead of "another chat operation", which is what made
the original warning read as a bug. Every outcome is at least as good as
before; the common one is strictly better, in that the Save just works.

The guard's shape now lives in one factory, _new_mutation_guard(), so
chat_library, autosave and the tests cannot drift apart on it. Its release
Event is constructed with no running loop on purpose - safe since 3.10, and
register_chat_library really is called outside a loop, which is the exact
hazard that broke R6.6's own create_task call.

Mutation-tested, and the first attempt failed the bar. The initial test used a
hand-written double that reimplemented the claim/release itself, so mutating
the REAL _guarded_tick - dropping its owner tag or its release signal - left
the test green; the production wiring was never under test. Rewritten to drive
the real register_autosave loop with a deliberately slow save, bounded by
asyncio.wait_for so a lost wakeup fails instead of merely running slow. That
exposed a second gap: the original test only ever observed the FIRST tick to
claim, so removing _guarded_tick's released.clear() also survived - yet without
it, every tick after the first wakes a waiter on a STALE signal and drops the
user's intent exactly as before. A fix that works once then quietly stops is
worse than no fix, so there is now a test pinning the steady state too. All 5
mutants killed, including both that initially survived.

Suites: 2375 pytest (+5), 1053 vitest unchanged (no frontend touched),
tsc/eslint/build clean, Qt burn-down gate unchanged at 152. The chat-library
and autosave files were also re-driven against a scratch copy of a real
~/.graphlink/chats.db - 5 real legacy chats still load and tick without a
spurious rewrite through the rewritten guard. Real chats.db verified untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dovvnloading
dovvnloading merged commit af618e9 into main Jul 26, 2026
2 checks passed
@dovvnloading
dovvnloading deleted the qt-removal/r6-autosave-yields-to-user branch July 26, 2026 01:49
dovvnloading added a commit that referenced this pull request Jul 26, 2026
test_a_user_save_waits_out_a_real_in_flight_autosave_tick_instead_of_being_dropped
failed once on main after #142 merged, then passed on a re-run of identical
code. It is a genuine flake, and the fragility is mine - I wrote this test
in #138.

Cause: the test raced the real autosave loop with interval_seconds=0.02
while each tick held the guard for ~0.1s. Because _loop sleeps AFTER a tick,
ticks ran effectively back-to-back, so the user's save had to win a
scheduling race in the narrow window between one tick releasing the guard
and the next re-claiming it. On top of that it asserted a 1.0s wall-clock
bound against a 2.0s production timeout - a 2x margin on a contended
2-core CI runner.

Fixed by removing the race rather than widening the bound:
- register_autosave now exposes bus.autosave_guarded_tick, the same
  testability seam already used for bus.autosave_task and
  bus.chat_save_state (the file's own comments justify exactly this: a
  closure-local is unreachable to the tests that must prove the wiring).
- The test drives exactly ONE real tick through that seam with
  interval_seconds=3600, so the periodic loop can never fire and no second
  tick can steal the guard back mid-handoff.
- The tick's duration is controlled by a gate instead of a sleep, so the
  user's save provably arrives while the guard is genuinely held.
- AUTOSAVE_YIELD_TIMEOUT_SECONDS is patched to 30s and the save asserted
  within 5s: a missing release signal still fails it, now with a 6x margin
  instead of 2x.

Still drives the REAL _guarded_tick, so it keeps catching what it exists to
catch - verified by mutation: dropping the AUTOSAVE owner tag fails it, and
dropping the released.set() signal fails it. Ran 25/25 green locally.

Full suite: 2438 passed, compileall clean.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant