fix(prefs): replace the desktop preference store atomically
Upstream shared_preferences_windows and _linux write the whole preference document with a bare `writeAsStringSync`. That opens with the default `FileMode.write`, which truncates the live file before writing it, so every single preference write has a window in which the only copy on disk is empty or half-written. A crash, power loss, forced reboot or antivirus interception inside that window leaves a document that fails to parse on every subsequent launch — and the store holds the credential-vault key, so the loss is not recoverable by rewriting it. This is the corruption class behind #1732; the recovery path already landed is a band-aid over it. Vendor both packages under packages/ — the convention saf_util and wakelock_plus already follow — and stage, flush, then rename over the target. The flush has to precede the rename or it could publish contents that were never committed, the same corruption by another route. Staging uses one fixed sibling name rather than a stamped one, because the file is a plaintext copy of the vault key, tracker refresh tokens and Seerr cookies; it is created in the target's own directory so rename stays on one volume and the mode matches what the canonical file would have had, and a stale one is swept once the canonical document has been read cleanly. Both deltas are marked in-source and in provenance.json with the refresh contract. Atomicity is proven, not asserted. A hard link to the store observes the old document after a write, which only holds when the directory entry was replaced — truncate-in-place would have rewritten the shared inode, and that test does fail against unpatched upstream. Upstream's own suites still pass unchanged in both packages and now run in CI, so the patch keeps the contract it inherited. Windows `MoveFileExW` replacement semantics cannot be proven on a POSIX runner or a memory file system, so they get their own test on the existing windows-latest job, including replacement while a reader holds the file open — antivirus and Search Indexer both do.
This commit is contained in:
edde746
@@ -0,0 +1,135 @@
|
||||
import 'dart:convert';
|
||||
import 'dart:io';
|
||||
|
||||
import 'package:flutter_test/flutter_test.dart';
|
||||
import 'package:path/path.dart' as p;
|
||||
import 'package:path_provider_linux/path_provider_linux.dart';
|
||||
import 'package:plezy/services/prefs_recovery.dart';
|
||||
import 'package:shared_preferences_linux/shared_preferences_linux.dart';
|
||||
import 'package:shared_preferences_platform_interface/types.dart';
|
||||
|
||||
/// The vendored `_writePreferences` patch, exercised against a real file
|
||||
/// system.
|
||||
///
|
||||
/// Upstream writes the whole preference document with a bare
|
||||
/// `writeAsStringSync`, which opens with the default `FileMode.write` and so
|
||||
/// truncates the live file before writing it. Every preference write therefore
|
||||
/// has a window in which the only copy on disk is empty or half-written, and
|
||||
/// the store holds the credential-vault key — the corruption behind #1732.
|
||||
///
|
||||
/// `shared_preferences_linux` and `shared_preferences_windows` carry the same
|
||||
/// patch and are byte-identical apart from their path-provider type, so the
|
||||
/// Linux copy stands in for both here. Windows rename semantics
|
||||
/// (`MoveFileExW` with MOVEFILE_REPLACE_EXISTING) cannot be proven on a POSIX
|
||||
/// host and are covered by the windows-latest step in CI.
|
||||
void main() {
|
||||
TestWidgetsFlutterBinding.ensureInitialized();
|
||||
|
||||
late Directory support;
|
||||
late File store;
|
||||
late File staging;
|
||||
late SharedPreferencesAsyncLinux backend;
|
||||
|
||||
const options = SharedPreferencesOptions();
|
||||
|
||||
setUp(() async {
|
||||
support = await Directory.systemTemp.createTemp('plezy_atomic_write_');
|
||||
store = File(p.join(support.path, prefsStoreFileName));
|
||||
staging = File('${store.path}.tmp');
|
||||
backend = SharedPreferencesAsyncLinux()..pathProvider = _TempPathProviderLinux(support.path);
|
||||
});
|
||||
|
||||
tearDown(() async {
|
||||
if (await support.exists()) await support.delete(recursive: true);
|
||||
});
|
||||
|
||||
test('a write leaves no staging file behind', () async {
|
||||
await backend.setString('theme', 'dark', options);
|
||||
|
||||
expect(await store.exists(), isTrue);
|
||||
expect(jsonDecode(await store.readAsString()), containsPair('theme', 'dark'));
|
||||
expect(await staging.exists(), isFalse);
|
||||
});
|
||||
test('a write replaces the document rather than rewriting it in place', () async {
|
||||
await backend.setString('theme', 'dark', options);
|
||||
|
||||
// A hard link names the same inode as the store. Under upstream's
|
||||
// truncate-then-write the link would observe the new content, because the
|
||||
// live file is rewritten underneath every reader holding it open — the
|
||||
// window that makes an interrupted write destroy the document. Under an
|
||||
// atomic rename the old inode is simply unlinked from the path, so the
|
||||
// witness keeps exactly what a reader mid-write would still have seen.
|
||||
final witness = File(p.join(support.path, 'witness.json'));
|
||||
if (!_run('ln', [store.path, witness.path])) {
|
||||
// Windows has no `ln` on PATH at all, and NTFS replacement is covered by
|
||||
// prefs_store_atomic_write_windows_test.dart instead.
|
||||
markTestSkipped('hard links are unavailable here');
|
||||
return;
|
||||
}
|
||||
|
||||
await backend.setString('theme', 'light', options);
|
||||
|
||||
expect(jsonDecode(await witness.readAsString()), containsPair('theme', 'dark'));
|
||||
expect(jsonDecode(await store.readAsString()), containsPair('theme', 'light'));
|
||||
});
|
||||
|
||||
test('an interrupted write leaves the live document valid', () async {
|
||||
await backend.setString('theme', 'dark', options);
|
||||
final intact = await store.readAsString();
|
||||
|
||||
// What an interruption leaves once writes are staged: a half-written
|
||||
// staging file that was never renamed. The invariant this guards is that
|
||||
// the staging file is never itself the live document.
|
||||
await staging.writeAsString('{"theme":"light","trunc', flush: true);
|
||||
|
||||
expect(await store.readAsString(), intact);
|
||||
expect(PrefsRecovery.describeStoreDamage(await store.readAsBytes()), isNull);
|
||||
});
|
||||
|
||||
test('a stale staging file is swept once the document reads cleanly', () async {
|
||||
await backend.setString('theme', 'dark', options);
|
||||
await staging.writeAsString('{"credential_vault_key_v1":"left-behind"', flush: true);
|
||||
|
||||
// The staging file is a plaintext copy of the credentials, so it must not
|
||||
// outlive the interrupted write that produced it.
|
||||
final reader = SharedPreferencesAsyncLinux()..pathProvider = _TempPathProviderLinux(support.path);
|
||||
await reader.getPreferences(const GetPreferencesParameters(filter: PreferencesFilters()), options);
|
||||
|
||||
expect(await staging.exists(), isFalse);
|
||||
expect(jsonDecode(await store.readAsString()), containsPair('theme', 'dark'));
|
||||
});
|
||||
|
||||
test('writing into a directory that does not exist yet still lands', () async {
|
||||
final nested = Directory(p.join(support.path, 'nested'));
|
||||
final nestedBackend = SharedPreferencesAsyncLinux()..pathProvider = _TempPathProviderLinux(nested.path);
|
||||
|
||||
await nestedBackend.setString('theme', 'dark', options);
|
||||
|
||||
expect(
|
||||
jsonDecode(await File(p.join(nested.path, prefsStoreFileName)).readAsString()),
|
||||
containsPair('theme', 'dark'),
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
class _TempPathProviderLinux extends PathProviderLinux {
|
||||
_TempPathProviderLinux(this.supportPath);
|
||||
|
||||
final String supportPath;
|
||||
|
||||
@override
|
||||
Future<String?> getApplicationSupportPath() async => supportPath;
|
||||
}
|
||||
|
||||
/// Runs a POSIX helper, reporting whether it succeeded.
|
||||
///
|
||||
/// Returns false rather than throwing when the binary is missing, so a Windows
|
||||
/// run reaches `markTestSkipped` instead of failing the whole suite on a
|
||||
/// ProcessException before the test body can decide anything.
|
||||
bool _run(String executable, List<String> arguments) {
|
||||
try {
|
||||
return Process.runSync(executable, arguments).exitCode == 0;
|
||||
} on ProcessException {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,99 @@
|
||||
@TestOn('windows')
|
||||
library;
|
||||
|
||||
import 'dart:convert';
|
||||
import 'dart:io';
|
||||
|
||||
import 'package:flutter_test/flutter_test.dart';
|
||||
import 'package:path/path.dart' as p;
|
||||
import 'package:path_provider_windows/path_provider_windows.dart';
|
||||
import 'package:plezy/services/prefs_recovery.dart';
|
||||
import 'package:shared_preferences_platform_interface/types.dart';
|
||||
import 'package:shared_preferences_windows/shared_preferences_windows.dart';
|
||||
|
||||
/// The vendored `shared_preferences_windows` atomic write, on real NTFS.
|
||||
///
|
||||
/// `prefs_store_atomic_write_test.dart` covers the same patch through the
|
||||
/// Linux twin and runs everywhere, but it can only prove POSIX `rename(2)`.
|
||||
/// The store this all exists to protect lives on Windows (#1732), and there
|
||||
/// the replacement goes through `MoveFileExW` with MOVEFILE_REPLACE_EXISTING —
|
||||
/// which a POSIX runner and a memory file system are both silent about. Run by
|
||||
/// the windows-native-test job in CI.
|
||||
void main() {
|
||||
TestWidgetsFlutterBinding.ensureInitialized();
|
||||
|
||||
late Directory support;
|
||||
late File store;
|
||||
late File staging;
|
||||
late SharedPreferencesAsyncWindows backend;
|
||||
|
||||
const options = SharedPreferencesOptions();
|
||||
|
||||
setUp(() async {
|
||||
support = await Directory.systemTemp.createTemp('plezy_atomic_write_win_');
|
||||
store = File(p.join(support.path, prefsStoreFileName));
|
||||
staging = File('${store.path}.tmp');
|
||||
backend = SharedPreferencesAsyncWindows()..pathProvider = _TempPathProviderWindows(support.path);
|
||||
});
|
||||
|
||||
tearDown(() async {
|
||||
if (await support.exists()) await support.delete(recursive: true);
|
||||
});
|
||||
|
||||
test('rename replaces an existing document on NTFS', () async {
|
||||
// The bare `MoveFileW` this would otherwise compile to fails outright when
|
||||
// the destination exists. If the vendored write ever loses its replace
|
||||
// semantics, every preference write after the first one fails silently —
|
||||
// `_writePreferences` swallows the exception and returns false.
|
||||
await backend.setString('theme', 'dark', options);
|
||||
await backend.setString('theme', 'light', options);
|
||||
|
||||
expect(jsonDecode(await store.readAsString()), containsPair('theme', 'light'));
|
||||
expect(await staging.exists(), isFalse);
|
||||
});
|
||||
|
||||
test('a replaced document is complete and parseable', () async {
|
||||
for (var i = 0; i < 25; i++) {
|
||||
await backend.setString('key$i', 'value$i', options);
|
||||
// Every intermediate state is a whole document, never a truncation.
|
||||
expect(PrefsRecovery.describeStoreDamage(await store.readAsBytes()), isNull);
|
||||
}
|
||||
|
||||
final document = jsonDecode(await store.readAsString()) as Map<String, dynamic>;
|
||||
expect(document, containsPair('key0', 'value0'));
|
||||
expect(document, containsPair('key24', 'value24'));
|
||||
});
|
||||
|
||||
test('a stale staging file is swept once the document reads cleanly', () async {
|
||||
await backend.setString('theme', 'dark', options);
|
||||
await staging.writeAsString('{"credential_vault_key_v1":"left-behind"', flush: true);
|
||||
|
||||
final reader = SharedPreferencesAsyncWindows()..pathProvider = _TempPathProviderWindows(support.path);
|
||||
await reader.getPreferences(const GetPreferencesParameters(filter: PreferencesFilters()), options);
|
||||
|
||||
expect(await staging.exists(), isFalse);
|
||||
});
|
||||
|
||||
test('an open reader does not block the replacement', () async {
|
||||
// Windows keeps mandatory locks on open handles, and antivirus and Search
|
||||
// Indexer both hold the store open. A replacement that a reader can veto
|
||||
// would turn every preference write into a silent no-op on exactly the
|
||||
// machines most likely to have damaged the store in the first place.
|
||||
await backend.setString('theme', 'dark', options);
|
||||
final handle = await store.open();
|
||||
addTearDown(handle.close);
|
||||
|
||||
await backend.setString('theme', 'light', options);
|
||||
|
||||
expect(jsonDecode(await store.readAsString()), containsPair('theme', 'light'));
|
||||
});
|
||||
}
|
||||
|
||||
class _TempPathProviderWindows extends PathProviderWindows {
|
||||
_TempPathProviderWindows(this.supportPath);
|
||||
|
||||
final String supportPath;
|
||||
|
||||
@override
|
||||
Future<String?> getApplicationSupportPath() async => supportPath;
|
||||
}
|
||||
Reference in New Issue
Block a user