Serialise sleep-state writers and reject corrupt state

[Ryan Duguid (ryanduguid)]

Summary

Acquire one process lock before either state-writing entry point loads state. Save through unique temporary files and refuse corrupt state rather than silently replacing it. A competing cycle stops before model work and can retry after the lock is released.

Evidence

• Before: Concurrent writers could load the same state and overwrite each other, and a failed state parse could discard existing progress.
  After: All eight focused Windows state/process-lock cases pass using fabricated state. They cover competing writers, lock release and abrupt process exit.

Risk

A concurrent cycle now fails explicitly instead of continuing with stale state.

Unverified

The full current engine suite, live transcript handling and real backend integrations were not rerun. No live home/session data was used.

[Yifan Yang (Yif-Yang)]

The locking change has useful production evidence: two real processes sharing state reject overlap before model work, a retry preserves both nights, and abrupt process exit releases the lock. Both existing state-writing entry points refuse contention before loading. The full Linux suite has 1,720 passes and 13 skips; its only failure is this audit sandbox's unavailable POSIX semaphore used by multiprocessing.Event. Independent subprocess tests avoid that facility and pass all three cases.

Please address the reader upgrade boundary before merge. The real existing writer can produce valid GBK/cp1252 state; this head's new UTF-8-only loader rejects both with UnicodeDecodeError on the same simulated locale. It does preserve the file, so this is not a data-overwrite report. Coordinate with #302 to retain legacy migration, or provide a deliberate, actionable migration diagnostic naming the state file. Keep the non-object rejection and transaction-lock tests. The current five-PR proposed subset merges with this head cleanly; text conflicts are not the reason for this hold. CLA and official CI remain separate gates.

[Ryan Duguid (ryanduguid)]

Thanks for testing this against real processes. b19ccff keeps the legacy migration: load() tries UTF-8 first, falls back to the locale codec the old writer used (so GBK and cp1252 files load), and the next save() writes UTF-8. A file that decodes under neither raises a ValueError naming the file and leaves it untouched. The non-object rejection and lock tests are unchanged; new tests cover GBK, cp1252 and the undecodable case.

This overlaps #302's reader. Happy to rebase onto it if that lands first.

[Ryan Duguid (ryanduguid)]

@microsoft-github-policy-service agree

[Yifan Yang (Yif-Yang)]

Thank you for restoring legacy-codec compatibility in b19ccffd7a0cc2a21731520661b391470ef453bc. The prior GBK/cp1252 rejection is resolved, and the independent real-process controls still demonstrate contention refusal, retry preserving both nights, and lock release after abrupt exit. CLA now passes as well.

#302 is now merged on main 1a44050fc53a. Keeping this as a Draft while you rebase and retain the locking work is appropriate. Please reuse the common state reader and its path-bearing diagnostics while retaining the lock and unique-tempfile behavior. The remaining malformed/non-object CLI checks exit safely but omit the state path; these can converge with #302 rather than becoming a second reader contract. If #322 is subsequently integrated, preserve its source-qualified checkpoints through both writer routes and rerun the concurrency, migration and handoff tests on the rebased head. The unavailable semaphore in one audit-runner test is an environment limitation, not a reproduced lock defect. This is an acknowledgment and integration request, not a Draft merge recommendation.

[Ryan Duguid (ryanduguid)]

Thanks. f49e15b merges main (with #302) instead of rebasing, so the review history stays intact. The branch now uses #302's _read_state and StateFileError for every load, so malformed and non-object state files are reported with the state path. Its own reader and the duplicate legacy-codec tests are gone. What remains is the transaction lock (both the cycle and the handoff routes) and the unique-tempfile save, with their concurrency and interrupted-save tests. The state, concurrency and engine tests pass locally. The only failures are the Windows-only test_cycle_stages_only_documents_that_changed cases, which also fail on main. I'll keep it as a draft and rerun these tests if #322 lands first.