close the fourth review's three findings on the 3.8.6 Doctor fixes

@@ -220,19 +220,21 @@220220 // undo_late_strips applies strips that reached the remote during an upload to221221 // what the upload put, and reports whether there were any. Records stay, less222222 // the stripped paths: another offer, or head itself, may already name them.223-// If this fails, Doctor's remote audit finds what is left and sends the strip224-// again once this repo holds it.223+// Each strip's note is written first, so if this stops part way, Doctor sends224+// the strip again once a steal brings it here. Objects go last, so no record225+// is left naming one that is gone.225226 fn undo_late_strips(store &storage.Store, mut backend remote.Backend, repo string, uploaded Uploaded) !bool {226227 	strips := unapplied_strips(store, mut backend, repo)!227228 	if strips.len == 0 {228229 		return false229230 	}230-	// Objects go first: a record that still lists the path is what lets231-	// Doctor's remote audit find the strip again if this stops part way.232-	drop_stripped_objects(store, mut backend, repo, uploaded, strips)!231+	for strip in strips {232+		queue_strip(store, strip.path, [strip.save_id], uploaded.objects.keys())!233+	}233234 	for id in uploaded.saves {234235 		strip_record(mut backend, save_key(repo, id), id, strips)!235236 	}237+	drop_stripped_objects(store, mut backend, repo, uploaded, strips)!236238 	return true237239 }238240 @@ -395,6 +397,9 @@395397 	for save in history.line(store)! {396398 		on_line[save.id] = true397399 	}400+	// Strips come first: an object only a stripped path named may be gone.401+	fetched.events_out = fetch_events(store, mut backend, repo)!402+	strips := known_strips(store, fetched)!398403 	mut cursor := fetched.head399404 	for cursor != '' && cursor !in on_line {400405 		mut record := if saved := history.by_id(store, cursor) {@@ -408,7 +413,8 @@408413 		}409414 		fetched.unsafe_paths << drop_unsafe_paths(mut record)410415 		for file in record.files {411-			if !store.has_object(file.hash)! && file.hash !in fetched.repo_objects {416+			if !store.has_object(file.hash)! && file.hash !in fetched.repo_objects417+				&& !strips.any(reaches(it, record.id) && covers(it.path, file.path)) {412418 				blob := backend.get(object_key(repo, file.hash))!413419 				if !blob.found {414420 					return error('the remote is missing object ${file.hash} that save ${record.id} needs')@@ -421,7 +427,6 @@421427 	}422428 	fetched.saves.reverse_in_place()423429 	fetched.base = cursor424-	fetched.events_out = fetch_events(store, mut backend, repo)!425430 	return fetched426431 }427432 

@@ -1124,6 +1124,9 @@11241124 	fail_after_race bool11251125 	raced_put       bool11261126 	fail_delete     bool1127+	fail_match      bool1128+	// race at the first object instead of the first save record1129+	on_objects bool11271130 }11281131 11291132 fn (mut r StripRace) get(key string) !remote.Object {@@ -1138,7 +1141,7 @@11381141 	if r.fail_after_race && r.raced_put {11391142 		return error('injected write failure')11401143 	}1141-	if !r.done && key.contains('/saves/') {1144+	if !r.done && key.contains(if r.on_objects { '/objects/' } else { '/saves/' }) {11421145 		r.done = true11431146 		r.raced_put = true11441147 		strip_local(r.other, r.path, '')!@@ -1148,6 +1151,9 @@11481151 }11491152 11501153 fn (mut r StripRace) put_if_match(key string, body []u8, tag string) !bool {1154+	if r.fail_match {1155+		return error('injected rewrite failure')1156+	}11511157 	return r.inner.put_if_match(key, body, tag)!11521158 }11531159 @@ -1322,48 +1328,111 @@13221328 		assert false, 'a note naming a path outside the objects must be refused'13231329 	}13241330 	assert os.read_file(victim)! == 'another repo'1331+	// Nor may a remote record's path-shaped hash be noted or deleted.1332+	a.remove_state(pending_key('app'))!1333+	requeue_strip(a, 'app')!1334+	backend.delete(save_key('proj', save.id))!1335+	backend.put_if_absent(save_key('proj', save.id), encode_save(SaveRecord{1336+		id:    save.id1337+		files: [FileRecord{1338+			path: 'app'1339+			hash: '../../victim/head'1340+		}]1341+	}).bytes())!1342+	send_strip(a, mut backend, 'app')!1343+	assert os.read_file(victim)! == 'another repo'13251344 }13261345 1327-// A settlement that stops part way leaves the record naming the stripped1328-// path, which is what lets Doctor send the strip again once this repo has it.1329-fn test_a_failed_settlement_leaves_residue_doctor_can_finish() {1330-	base := scratch('settle_fail')!1346+// A settlement that stops at a record leaves it naming an object the strip1347+// deleted. A machine behind must still steal past it, and the note written1348+// first finishes the strip once this repo has it.1349+fn test_a_settlement_stopped_at_a_record_keeps_history_readable() {1350+	base := scratch('settle_record')!13311351 	defer {13321352 		os.rmdir_all(base) or {}13331353 	}13341354 	mut backend := remote.new_folder(os.join_path(base, 'remote'))!1335-	mut a := open_repo(machine('settle_fail_a')!)!1355+	mut a := open_repo(machine('settle_record_a')!)!13361356 	defer {13371357 		a.close()13381358 	}1339-	mut b := open_repo(machine('settle_fail_b')!)!1359+	mut b := open_repo(machine('settle_record_b')!)!13401360 	defer {13411361 		b.close()13421362 	}1363+	mut c := open_repo(machine('settle_record_c')!)!1364+	defer {1365+		c.close()1366+	}1367+	put(a.root, 'readme.txt', 'hello\n')!1368+	make_save(a, 'start')!1369+	offer_now(a, mut backend)!1370+	join(c, mut backend)!13431371 	put(a.root, 'secrets.env', 'api_key=1\n')!1344-	first := make_save(a, 'leak the key')!1372+	leak := make_save(a, 'leak the key')!13451373 	offer_now(a, mut backend)!13461374 	join(b, mut backend)!13471375 	secret := object_key('proj', storage.hash_hex('api_key=1\n'.bytes()))1348-	backend.delete(save_key('proj', first.id))!1376+	backend.delete(save_key('proj', leak.id))!13491377 	backend.delete(secret)!13501378 	mut race := StripRace{1379+		inner:      backend1380+		other:      &b1381+		path:       'secrets.env'1382+		fail_match: true1383+	}1384+	if _ := resend(a, mut race, 'proj', [], [leak.id]) {1385+		assert false, 'the injected rewrite failure should have surfaced'1386+	}1387+	assert 'secrets.env' in decode_save(backend.get(save_key('proj', leak.id))!.body)!.files.map(it.path)1388+	steal_now(c, mut backend)!1389+	assert history.head(c) or { history.Save{} }.id == leak.id1390+	assert strip_pending(a, 'secrets.env')1391+	steal_now(a, mut backend)!1392+	send_strip(a, mut backend, 'secrets.env')!1393+	cleaned := decode_save(backend.get(save_key('proj', leak.id))!.body)!1394+	assert 'secrets.env' !in cleaned.files.map(it.path)1395+	assert !backend.get(secret)!.found1396+}1397+1398+// With no record to leave behind, the note alone carries a resent object that1399+// a strip removed meanwhile, until a send after steal deletes it.1400+fn test_an_object_only_resend_leaves_a_note_that_finishes_after_steal() {1401+	base := scratch('settle_object')!1402+	defer {1403+		os.rmdir_all(base) or {}1404+	}1405+	mut backend := remote.new_folder(os.join_path(base, 'remote'))!1406+	mut a := open_repo(machine('settle_object_a')!)!1407+	defer {1408+		a.close()1409+	}1410+	mut b := open_repo(machine('settle_object_b')!)!1411+	defer {1412+		b.close()1413+	}1414+	put(a.root, 'secrets.env', 'api_key=1\n')!1415+	make_save(a, 'leak the key')!1416+	offer_now(a, mut backend)!1417+	join(b, mut backend)!1418+	hash := storage.hash_hex('api_key=1\n'.bytes())1419+	backend.delete(object_key('proj', hash))!1420+	mut race := StripRace{13511421 		inner:       backend13521422 		other:       &b13531423 		path:        'secrets.env'13541424 		fail_delete: true1425+		on_objects:  true13551426 	}1356-	if _ := resend(a, mut race, 'proj', [], [first.id]) {1427+	if _ := resend(a, mut race, 'proj', [hash], []) {13571428 		assert false, 'the injected delete failure should have surfaced'13581429 	}1359-	record := decode_save(backend.get(save_key('proj', first.id))!.body)!1360-	assert 'secrets.env' in record.files.map(it.path)1430+	assert backend.get(object_key('proj', hash))!.found1431+	assert strip_pending(a, 'secrets.env')13611432 	steal_now(a, mut backend)!1362-	requeue_strip(a, 'secrets.env')!13631433 	send_strip(a, mut backend, 'secrets.env')!1364-	cleaned := decode_save(backend.get(save_key('proj', first.id))!.body)!1365-	assert 'secrets.env' !in cleaned.files.map(it.path)1366-	assert !backend.get(secret)!.found1434+	assert !backend.get(object_key('proj', hash))!.found1435+	assert !strip_pending(a, 'secrets.env')13671436 }13681437 13691438 // A strip whose deletes fail after its rewrite keeps, in its note, the objects

@@ -15,6 +15,7 @@1515 /// in one transaction.1616 pub fn fetch_missing(store &storage.Store, mut backend remote.Backend, repo string, ids []string) !int {1717 	mut fetched := Fetched{}18+	strips := known_strips(store, fetched)!1819 	mut wanted := ids.clone()1920 	mut seen := map[string]bool{}2021 	for wanted.len > 0 {@@ -37,7 +38,8 @@3738 			return error('remote save ${id} has invalid metadata')3839 		}3940 		for file in record.files {40-			if !store.has_object(file.hash)! && file.hash !in fetched.repo_objects {41+			if !store.has_object(file.hash)! && file.hash !in fetched.repo_objects42+				&& !strips.any(reaches(it, id) && covers(it.path, file.path)) {4143 				blob := backend.get(object_key(repo, file.hash))!4244 				if !blob.found {4345 					return error('the remote is missing object ${file.hash} that save ${id} needs')

@@ -65,7 +65,7 @@6565 	if note.saves.len == 0 || !note.saves.all(fn [recorded] (scope string) bool {6666 		return recorded.any(it.save_id == scope)6767 	}) {68-		return error('no recorded strip matches the unfinished strip note for ${path}; it is kept for recovery')68+		return error('no recorded strip matches the unfinished strip note for ${path}; it is kept for recovery, and koh steal brings a strip made elsewhere')6969 	}7070 	return note7171 }@@ -140,12 +140,7 @@140140 	events.strip(store, only_save, path)!141141 	// An earlier strip of the same path may not have finished on the remote,142142 	// so the note grows rather than resets.143-	mut note := read_pending(store, path)!144-	if only_save !in note.saves {145-		note.saves << only_save146-	}147-	note.hashes << hashes148-	store.set_state(pending_key(path), json2.encode(note))!143+	queue_strip(store, path, [only_save], hashes)!149144 	return LocalStrip{150145 		files_removed:   removed151146 		objects_dropped: dropped@@ -170,7 +165,7 @@170165 }171166 172167 fn send_steps(store &storage.Store, mut backend remote.Backend, path string, mut report SendReport) ! {173-	mut note := read_pending(store, path)!168+	note := read_pending(store, path)!174169 	name := repo_name(store, mut backend)!175170 	mut upload := OfferReport{176171 		repo: name@@ -205,18 +200,14 @@205200 		}206201 		// Once rewritten, the record no longer names these objects; the note207202 		// keeps them so a retry can still delete them.208-		unnoted := dropped.filter(it !in note.hashes)209-		if unnoted.len > 0 {210-			note.hashes << unnoted211-			store.set_state(pending_key(path), json2.encode(note))!212-		}203+		queue_strip(store, path, []string{}, dropped)!213204 		if !backend.put_if_match(key, encode_save(rewrite(record, kept)).bytes(), remote_object.tag)! {214205 			return error('the remote copy of save ${record.id} moved while stripping — run koh steal, then strip again')215206 		}216207 		report.records++217208 	}218209 	for hash, _ in freed {219-		if hash in needed || store.has_object(hash)! {210+		if hash in needed || !storage.valid_hash(hash) || store.has_object(hash)! {220211 			continue221212 		}222213 		backend.delete(object_key(name, hash))!@@ -228,17 +219,23 @@228219 // requeue_strip notes every valid recorded strip of `path` again, so that229220 // send_strip removes the path from remote records that still hold it.230221 pub fn requeue_strip(store &storage.Store, path string) ! {231-	mut note := read_pending(store, path)!232222 	recorded := events.strip_events(store)!.filter(events.valid_strip(it) && it.path == path)233223 	if recorded.len == 0 {234224 		return error('this repo has no recorded strip of ${path}; run koh steal, then koh doctor')235225 	}236-	for event in recorded {237-		if event.save_id !in note.saves {238-			note.saves << event.save_id239-		}226+	queue_strip(store, path, recorded.map(it.save_id), []string{})!227+}228+229+// queue_strip adds scopes and objects to the note send_strip works from,230+// writing it only when something is new. Only object hashes are noted.231+fn queue_strip(store &storage.Store, path string, scopes []string, hashes []string) ! {232+	mut note := read_pending(store, path)!233+	count := note.saves.len + note.hashes.len234+	note.saves << scopes.filter(it !in note.saves)235+	note.hashes << hashes.filter(it !in note.hashes && storage.valid_hash(it))236+	if note.saves.len + note.hashes.len > count {237+		store.set_state(pending_key(path), json2.encode(note))!240238 	}241-	store.set_state(pending_key(path), json2.encode(note))!242239 }243240 244241 // unapplied_strips are the strip events on the remote this repo has not