fix(sessions): close the snapshot check/use race and guard damaged state_meta

[teknium1]

Summary

Post-merge follow-up to #71770. @helix4u posted review findings 47 seconds before I merged; I missed them. Both reproduced against merged main, both fixed here.

1. Check/use race in _copy_source_bundle (my bug)

The guard I added in #71770 called has_live_connection(), released the registry lock, and then ran shutil.copy2() over the bundle. A tracked connection could open in that window, and the copy's close() cancels its POSIX advisory locks — the exact class #71724 closed.

Measured on merged main, racer thread opening a connection mid-copy:

	before	after
connection opened during copy	yes, blocked 0.000s	blocked 10.0s until copy released
race window	open	closed

Adds sqlite_safe_read.offline_file_access() — a context manager holding the connection-lifecycle lock across an entire multi-step raw access — and routes the bundle copy through it. Future raw reads of a database file (hashing, moving a bundle aside) should use this instead of a bare pre-check.

2. _copy_state_meta_salvage assumed a key column

A damaged state_meta can retain value and lose key; columns.index("key") then raised ValueError and aborted the whole partial recovery. The mirror case (key without value) would have copied key-only rows and reported the table complete.

before: ValueError: tuple.index(x): x not in tuple   <- aborts recovery
after:  {'status': 'missing'}                        <- matches _copy_state_meta

Now requires both columns, matching the non-partial path, so an unusable optional table is recorded as missing/failed and --allow-partial still recovers sessions and messages.

Validation

• 937 targeted tests green (recovery, lock-safety, state, backup, WAL gate, kanban, malformed-repair, WAL fallback); ruff clean.
• Sabotage-verified: reinstating the bare pre-check fails the race test; removing the key/value requirement fails the state_meta test.
• Original reproduction script re-run post-fix: both findings closed.

Credit to @helix4u for catching both.

[github-actions]

૮ >ﻌ< ა ci review

_{ran on 8e74db6}

all good!

[helix4u]

@teknium1
The production fix for the check/use race looks correct. offline_file_access() holds the same lifecycle lock used by connect_tracked() across the complete main/WAL/SHM/journal copy, so another tracked connection cannot open between the check and the raw file closes.

Two things are still left:

1. Damaged state_meta is not reported as data loss.

When state_meta exists but lacks key or value, _copy_state_meta_salvage() returns status: "missing". _verify_recovered_database() only turns failed and partial statuses into warnings and loss_detected; it ignores missing.

The current reproduction can therefore recover sessions and messages while reporting:

• no warning about the lost metadata
• loss_detected = false
• partial = false
• potentially complete = true

The crash and key-only copy are fixed, but the report can now claim completeness after dropping an existing damaged metadata table. Since the table exists but is unusable, this should return status: "failed". The existing optional-table verification path will then keep the output structurally verified while setting a warning, loss_detected=true, partial=true, and complete=false.

The regression should assert those report fields rather than accepting either missing or failed.

2. The new race test has its own scheduling race.

After _copy_source_bundle() releases the lifecycle lock, the racer can acquire it before the main test thread calls release_copy.set(). In that schedule, copy_finished_first becomes false even though the production guard worked. The test also waits ten seconds on the successful path.

Please run the copy in a worker thread, pause it inside the patched copy operation, attempt connect_tracked() from the test thread or a second worker, assert that it remains blocked, then release the copy and assert that the connection opens afterward. That directly tests the lock ordering without relying on which thread runs immediately after the guard releases.

The underlying lock fix is right. These are the remaining reporting and regression-test issues I found.

[teknium1]

Both correct again, and the first one is worse than you flagged — my "fix" converted a loud crash into silent data loss. Measured on the branch before changing anything:

state_meta copy status : missing
verification warnings  : []
loss_detected          : False
partial                : False
complete               : True     <- claimed clean after dropping real metadata

A ValueError that aborts recovery is bad. Reporting complete=True on a database that quietly lost its metadata table is worse — the user has no signal at all. You called the mechanism exactly: verification only escalates failed/partial, so missing was invisible to it.

Now split by cause rather than symptom:

• present but unusable → failed → warnings=['state_meta copy status is failed'], loss_detected=True, partial=True, complete=False, verified=True (structurally sound, installable-with-review)
• genuinely absent → missing, no warning — nothing was lost

2. Race test rewritten to your design. You were right that it was schedule-dependent: after the guard released the lock, the racer could acquire it before the main thread set release_copy, failing on a correct implementation. Now the copy runs in a worker parked inside the patched operation, a second worker attempts connect_tracked(), the test asserts it stays blocked, releases, then asserts it opens. Deterministic, and ~1.1s instead of 10s. Ran it 12× — 12/12 stable.

One thing worth flagging from the sabotage run. I verify new tests by reinstating the defect and confirming they fail. The damaged-table and race sabotages both failed correctly. The absent-table one passed — meaning that test proved nothing. Cause: recover_session_database short-circuits on the inspection result when state_meta is entirely absent, so the helper's absent-branch is never reached through the public path and a regression there is invisible end-to-end. Added a unit-level test calling _copy_state_meta_salvage directly to pin both statuses. Now all three sabotages fail as they should.

That's the second time this round that a test of mine looked fine and was hollow. Worth naming since it's the same class of thing you caught in the race test.

939 targeted tests green, ruff clean. Thanks — three real defects across two rounds, and the reporting one would have been genuinely harmful to someone recovering a damaged database.

[helix4u]

That addresses both findings. The state_meta reporting and assertions look right now, and the revised race test is a substantial improvement.

There is technically still a small scheduler-dependent false-pass window because connection_opened.wait(1.0) does not prove the connector reached the lock attempt. Given that the removed-lock sabotage failed consistently across 12 runs, I would not hold this PR up over it. Just noting it in case the concurrency test is hardened later.

[teknium1]

Thanks — and I took the false-pass window anyway, since it was two lines.

You were right that connection_opened.wait(1.0) proved the connection hadn't opened but not that the connector had reached connect_tracked(); an unscheduled thread produces the same observation. The connector now sets a connect_attempted event immediately before the blocking call, and the test waits on that first, so "still blocked" means blocked at the lock rather than not yet started.

15/15 stable at ~1.1s, removed-lock sabotage still fails, and the branch is rebased onto current main (it had drifted 4 commits behind during the review rounds).

Appreciate the three rounds. Recap of what you caught, since it's a good sequence: a check/use race I introduced while fixing a check/use race; a ValueError that aborted recovery; then my fix for that one silently reporting complete=True after dropping real metadata. The last was the most dangerous and the least visible.

[helix4u]

Lol, fair enough. Technically, the connector could still be preempted in the tiny interval between connect_attempted.set() and actually entering connect_tracked(), so this is not a mathematical proof that it has reached the lock. That window is narrow enough that replacing the lock with an instrumented test wrapper would just be test-lawyering at this point.

This closes the meaningful false-pass case I was pointing at. Between this, the corrected state_meta reporting and assertions, and the rebase onto current main, I do not have anything else. Looks good from my side pending CI.