close the second review's five findings on the 3.8.6 Doctor fixes

@@ -9,6 +9,7 @@99 import json21010 import os1111 import time12+import src.exchange1213 import src.fmt1314 import src.history1415 import src.storage@@ -62,7 +63,7 @@6263 			attempted = true6364 			repair.run() or {6465 				repair_errors << '${repair.action}: ${err.msg()}'65-				if err is PartialRepair {66+				if err is exchange.Partway {6667 					repaired << err.done6768 				}6869 				continue@@ -427,6 +428,8 @@427428 	damaged_objects      []string428429 	missing_saves        []string429430 	strip_residue        bool431+	strip_residue_paths  []string432+	remote_strip_residue []string430433 	remote_bad_objects   []string431434 	remote_missing_saves []string432435 }

@@ -144,6 +144,7 @@144144 	assert report.flagged_saves == 1145145 	assert report.problems.any(it.contains('unknown kind'))146146 	assert report.problems.any(it.contains('stripped path'))147+	assert report.strip_residue_paths == ['private']147148 }148149 149150 struct DoctorMemory {@@ -377,8 +378,8 @@377378 	if _ := repair_objects(store, [kept, lost], '', '') {378379 		assert false, 'an object with no verified copy must fail its repair'379380 	} else {380-		assert err is PartialRepair381-		assert (err as PartialRepair).done == 'restored 1 damaged or missing object'381+		assert err is exchange.Partway382+		assert (err as exchange.Partway).done == 'restored 1 damaged or missing object'382383 		assert err.msg().contains(lost)383384 	}384385 	assert store.get_object(kept)! == 'kept'.bytes()@@ -480,8 +481,8 @@480481 	if _ := repair_objects(store, [kept, lost], '', '') {481482 		assert false, 'an unreachable remote must fail the repair'482483 	} else {483-		assert err is PartialRepair484-		assert (err as PartialRepair).done == 'restored 1 damaged or missing object'484+		assert err is exchange.Partway485+		assert (err as exchange.Partway).done == 'restored 1 damaged or missing object'485486 		assert err.msg().contains('not there'), err.msg()486487 	}487488 }@@ -513,4 +514,45 @@513514 	})!514515 	assert exchange.fetch_missing(store, mut backend, 'repo', ['aaaaaaaaaaaa'])! == 1515516 	assert history.files_of(store, 'aaaaaaaaaaaa')!.map(it.path) == ['app']517+}518+519+// A strip whose remote half was lost is sent again from the recorded event520+// when the remote still holds the path; a later path that fails still reports521+// the strips already sent.522+fn test_doctor_sends_a_recorded_strip_again_for_remote_residue() {523+	store, dir := doctor_store('remote_residue')!524+	remote_dir := os.join_path(dir, 'remote')525+	defer {526+		store.close()527+		machine.forget_remote() or {}528+		os.rmdir_all(dir) or {}529+	}530+	secret := 'secret'.bytes()531+	hash := storage.hash_hex(secret)532+	mut folder := remote.new_folder(remote_dir)!533+	folder.put_if_absent(exchange.save_key('repo', 'aaaaaaaaaaaa'), json2.encode(exchange.SaveRecord{534+		id:    'aaaaaaaaaaaa'535+		files: [exchange.FileRecord{ path: 'app', hash: hash }]536+	}).bytes())!537+	folder.put_if_absent(exchange.object_key('repo', hash), zlib.compress(secret)!)!538+	machine.save_remote(machine.RemoteSettings{ kind: machine.kind_folder, path: remote_dir })!539+	store.set_state('remote_repo', 'repo')!540+	events.strip(store, '', 'app')!541+	store.set_state('strip_pending:bad', 'not JSON')!542+	report := DoctorJson{543+		remote_repo:          'repo'544+		remote_strip_residue: ['app', 'bad']545+	}546+	repairs := plan_repairs(store, report).filter(it.action.contains('remote still holds'))547+	assert repairs.len == 1548+	if _ := repairs[0].run() {549+		assert false, 'an untrusted note must stop the second path'550+	} else {551+		assert err is exchange.Partway552+		assert (err as exchange.Partway).done == 'sent 1 strip', (err as exchange.Partway).done553+	}554+	record := exchange.decode_save(folder.get(exchange.save_key('repo', 'aaaaaaaaaaaa'))!.body)!555+	assert record.files.len == 0556+	assert !folder.get(exchange.object_key('repo', hash))!.found557+	assert !exchange.strip_pending(store, 'app')516558 }

@@ -86,6 +86,9 @@8686 				&& (file.path == event.path || file.path.starts_with('${event.path.trim_right('/')}/')) {8787 				report.problems << 'save ${id} still contains stripped path ${snapshot.escape_path(file.path)}'8888 				report.strip_residue = true89+				if event.path !in report.strip_residue_paths {90+					report.strip_residue_paths << event.path91+				}8992 			}9093 		}9194 	}

@@ -125,6 +125,7 @@125125 	for save in saves {126126 		doctor_files(save.id, files[save.id], event_list, mut remote_report)127127 	}128+	report.remote_strip_residue = remote_report.strip_residue_paths128129 	for hash in hashes.keys() {129130 		if !storage.valid_hash(hash) {130131 			remote_report.problems << 'invalid remote object hash ${hash}'

@@ -78,14 +78,17 @@7878 	if report.pending_strips.len > 0 && writable_remote {7979 		paths := report.pending_strips8080 		repairs << DoctorRepair{'send ${fmt.plural(paths.len, 'unfinished strip')} to the remote', fn [store, paths, remote_repo] () ! {81-			mut backend, _ := doctor_backend(store, '', remote_repo)!82-			for index, path in paths {83-				exchange.send_strip(store, mut backend, path) or {84-					return partial('sent', index, 'unfinished strip', err)85-				}86-			}81+			send_strips(store, remote_repo, paths, false)!8782 		}}8883 	}84+	// A strip whose remote half failed after the note was gone, as when an85+	// upload raced it, is sent again from the recorded event.86+	if report.remote_strip_residue.len > 0 && writable_remote {87+		paths := report.remote_strip_residue88+		repairs << DoctorRepair{'remove ${fmt.plural(paths.len, 'stripped path')} the remote still holds', fn [store, paths, remote_repo] () ! {89+			send_strips(store, remote_repo, paths, true)!90+		}}91+	}8992 	// With damaged rows an unused object may be the only copy of saved content.9093 	if report.unreferenced_objects > 0 && report.problems.len == 0 {9194 		repairs << DoctorRepair{'delete ${fmt.plural(int(report.unreferenced_objects), 'object')} no save uses', fn [store] () ! {@@ -110,25 +113,14 @@110113 	return repairs111114 }112115 113-// PartialRepair is a repair that wrote some of its changes before it failed;114-// Doctor reports both halves.115-struct PartialRepair {116-	Error117-	done   string118-	reason string119-}120-121-fn (e PartialRepair) msg() string {122-	return e.reason123-}124-125116 // Each copy must hash back to its key before it replaces anything: first the126117 // stored bytes (only the size was wrong), then working files, then the remote.127118 fn repair_objects(store &storage.Store, hashes []string, source string, remote_repo string) ! {128119 	mut lost := []string{}129120 	for index, hash in hashes {130121 		restored := restore_object(store, hash, source, remote_repo) or {131-			return partial('restored', index - lost.len, 'damaged or missing object', err)122+			return exchange.partway(done_text('restored', index - lost.len, 'damaged or missing object'),123+				err)132124 		}133125 		if !restored {134126 			lost << hash@@ -136,19 +128,28 @@136128 	}137129 	if lost.len > 0 {138130 		failure := error('no verified copy here or on the remote of ${lost.join(', ')}')139-		return partial('restored', hashes.len - lost.len, 'damaged or missing object', failure)131+		return exchange.partway(done_text('restored', hashes.len - lost.len, 'damaged or missing object'),132+			failure)140133 	}141134 }142135 143-// partial reports the `done` items a repair finished before `failure`, if any.144-fn partial(verb string, done int, noun string, failure IError) IError {145-	if done == 0 {146-		return failure136+fn send_strips(store &storage.Store, remote_repo string, paths []string, requeue bool) ! {137+	mut backend, _ := doctor_backend(store, '', remote_repo)!138+	for index, path in paths {139+		if requeue {140+			exchange.requeue_strip(store, path) or {141+				return exchange.partway(done_text('sent', index, 'strip'), err)142+			}143+		}144+		exchange.send_strip(store, mut backend, path) or {145+			return exchange.partway(done_text('sent', index, 'strip'), err)146+		}147147 	}148-	return PartialRepair{149-		done:   '${verb} ${fmt.plural(done, noun)}'150-		reason: failure.msg()151-	}148+}149+150+// done_text is what a repair finished before it failed, if anything.151+fn done_text(verb string, done int, noun string) []string {152+	return if done == 0 { []string{} } else { ['${verb} ${fmt.plural(done, noun)}'] }152153 }153154 154155 fn restore_object(store &storage.Store, hash string, source string, remote_repo string) !bool {

@@ -12,6 +12,7 @@1212 1313 import os1414 import src.events15+import src.fmt1516 import src.history1617 import src.remote1718 import src.snapshot@@ -138,12 +139,12 @@138139 	}139140 	mut uploaded := Uploaded{}140141 	upload_line(store, mut backend, name, local.id, remote_head, mut report, mut uploaded) or {141-		return after_failed_upload(store, mut backend, name, uploaded, false, err)142+		return after_failed_upload(store, mut backend, name, uploaded, err)142143 	}143144 	// A strip sends its event before it lists the saves to rewrite, so one144145 	// missed here lists them after these records landed and rewrites them. One145146 	// seen here may have rewritten before they landed: take back what it reaches.146-	if undo_late_strips(store, mut backend, name, uploaded, false)! {147+	if undo_late_strips(store, mut backend, name, uploaded)! {147148 		return error(strip_not_applied)148149 	}149150 	if !backend.put_if_match(head_key(name), local.id.bytes(), remote_head.tag)! {@@ -180,6 +181,14 @@180181 	objects map[string]bool181182 }182183 184+fn (u Uploaded) done() []string {185+	return if u.saves.len + u.objects.len == 0 {186+		[]string{}187+	} else {188+		['uploaded ${fmt.plural(u.objects.len, 'object')} and ${fmt.plural(u.saves.len, 'save record')}']189+	}190+}191+183192 // upload_line sends every save after the remote head, then the events.184193 fn upload_line(store &storage.Store, mut backend remote.Backend, name string, local string, remote_head remote.Object, mut report OfferReport, mut uploaded Uploaded) ! {185194 	if remote_head.found {@@ -201,43 +210,78 @@201210 202211 // after_failed_upload still applies a strip that landed before the failure203212 // to what the upload had put.204-fn after_failed_upload(store &storage.Store, mut backend remote.Backend, repo string, uploaded Uploaded, keep_records bool, failure IError) IError {205-	undo_late_strips(store, mut backend, repo, uploaded, keep_records) or {213+fn after_failed_upload(store &storage.Store, mut backend remote.Backend, repo string, uploaded Uploaded, failure IError) IError {214+	undo_late_strips(store, mut backend, repo, uploaded) or {206215 		return error('${failure.msg()}; removing what a strip on the remote covers also failed: ${err.msg()}')207216 	}208217 	return failure209218 }210219 211220 // undo_late_strips applies strips that reached the remote during an upload to212-// what the upload put, and reports whether there were any. An offer's records213-// go, since head never reached them. Doctor's resent records may already be214-// named by head, so they keep everything but the stripped paths; one that215-// moved meanwhile was rewritten by the strip itself.216-fn undo_late_strips(store &storage.Store, mut backend remote.Backend, repo string, uploaded Uploaded, keep_records bool) !bool {221+// what the upload put, and reports whether there were any. Records stay, less222+// 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.225+fn undo_late_strips(store &storage.Store, mut backend remote.Backend, repo string, uploaded Uploaded) !bool {217226 	strips := unapplied_strips(store, mut backend, repo)!218227 	if strips.len == 0 {219228 		return false220229 	}221230 	for id in uploaded.saves {222-		key := save_key(repo, id)223-		if !keep_records {224-			backend.delete(key)!225-			continue226-		}231+		strip_record(mut backend, save_key(repo, id), id, strips)!232+	}233+	drop_stripped_objects(store, mut backend, repo, uploaded, strips)!234+	return true235+}236+237+// strip_record rewrites one remote record without what `strips` cover. A238+// record that moved meanwhile is read again, since the other writer may have239+// kept a path these strips name.240+fn strip_record(mut backend remote.Backend, key string, id string, strips []events.Event) ! {241+	for _ in 0 .. 5 {227242 		object := backend.get(key)!228243 		if !object.found {229-			continue244+			return230245 		}231246 		record := decode_save(object.body)!232247 		kept := record.files.filter(fn [strips, id] (file FileRecord) bool {233248 			return !strips.any(reaches(it, id) && covers(it.path, file.path))234249 		})235-		if kept.len < record.files.len {236-			backend.put_if_match(key, encode_save(rewrite(record, kept)).bytes(), object.tag)!250+		if kept.len == record.files.len251+			|| backend.put_if_match(key, encode_save(rewrite(record, kept)).bytes(), object.tag)! {252+			return237253 		}238254 	}239-	drop_stripped_objects(store, mut backend, repo, uploaded, strips)!240-	return true255+	return error('the remote copy of save ${id} kept changing while a stripped path was removed; run koh doctor again')256+}257+258+// Partway is a failure after some work had already landed. `done` says what,259+// so Doctor can report both.260+pub struct Partway {261+	Error262+pub:263+	done   string264+	reason string265+}266+267+pub fn (e Partway) msg() string {268+	return e.reason269+}270+271+// partway adds what was `done` before `failure` to it, including what a272+// failing step of its own had done.273+pub fn partway(done []string, failure IError) IError {274+	mut all := done.clone()275+	if failure is Partway {276+		all << failure.done277+	}278+	if all.len == 0 {279+		return failure280+	}281+	return Partway{282+		done:   all.join('; ')283+		reason: failure.msg()284+	}241285 }242286 243287 // drop_stripped_objects deletes what an upload put that a strip reaches in a

@@ -1190,6 +1190,8 @@11901190 	}11911191 	assert backend.get(head_key('proj'))!.body.bytestr() == first.id11921192 	assert !backend.get(object_key('proj', secret))!.found1193+	// Another offer may already have published it, so the record stays, stripped.1194+	assert backend.get(save_key('proj', later.id))!.found11931195 	for key in backend.list('proj/saves')! {11941196 		record := decode_save(backend.get(key)!.body)!11951197 		assert 'secrets.env' !in record.files.map(it.path), record.id@@ -1314,6 +1316,122 @@13141316 		assert false, 'a note naming a path outside the objects must be refused'13151317 	}13161318 	assert os.read_file(victim)! == 'another repo'1319+}1320+1321+// MovingRecord stands in for another machine rewriting a record between this1322+// machine's read and its conditional write: the first conditional write finds1323+// `other` already removed from the record.1324+struct MovingRecord {1325+mut:1326+	inner remote.Backend1327+	other string1328+	moved bool1329+}1330+1331+fn (mut m MovingRecord) get(key string) !remote.Object {1332+	return m.inner.get(key)!1333+}1334+1335+fn (mut m MovingRecord) list(prefix string) ![]string {1336+	return m.inner.list(prefix)!1337+}1338+1339+fn (mut m MovingRecord) put_if_absent(key string, body []u8) !bool {1340+	return m.inner.put_if_absent(key, body)!1341+}1342+1343+fn (mut m MovingRecord) put_if_match(key string, body []u8, tag string) !bool {1344+	if !m.moved {1345+		m.moved = true1346+		current := m.inner.get(key)!1347+		record := decode_save(current.body)!1348+		other := record.files.filter(it.path != m.other)1349+		m.inner.put_if_match(key, encode_save(rewrite(record, other)).bytes(), current.tag)!1350+	}1351+	return m.inner.put_if_match(key, body, tag)!1352+}1353+1354+fn (mut m MovingRecord) delete(key string) ! {1355+	m.inner.delete(key)!1356+}1357+1358+fn test_a_stripped_record_that_moves_is_read_again() {1359+	base := scratch('moving_record')!1360+	defer {1361+		os.rmdir_all(base) or {}1362+	}1363+	mut folder := remote.new_folder(os.join_path(base, 'remote'))!1364+	key := save_key('proj', 'aaaaaaaaaaaa')1365+	folder.put_if_absent(key, encode_save(SaveRecord{1366+		id:    'aaaaaaaaaaaa'1367+		files: [FileRecord{1368+			path: 'secret-a'1369+		}, FileRecord{1370+			path: 'secret-b'1371+		}, FileRecord{1372+			path: 'keep'1373+		}]1374+	}).bytes())!1375+	mut moving := MovingRecord{1376+		inner: folder1377+		other: 'secret-b'1378+	}1379+	strip_record(mut moving, key, 'aaaaaaaaaaaa', [1380+		events.Event{1381+			kind: events.kind_strip1382+			path: 'secret-a'1383+		},1384+	])!1385+	assert decode_save(folder.get(key)!.body)!.files.map(it.path) == ['keep']1386+}1387+1388+// An invalid strip event on the remote cannot make an upload take anything back.1389+fn test_an_invalid_remote_strip_event_is_not_unapplied() {1390+	base := scratch('invalid_remote_strip')!1391+	defer {1392+		os.rmdir_all(base) or {}1393+	}1394+	mut backend := remote.new_folder(os.join_path(base, 'remote'))!1395+	mut a := open_repo(machine('invalid_remote_strip_a')!)!1396+	defer {1397+		a.close()1398+	}1399+	bad := events.Event{1400+		id:         'badstripevnt'1401+		kind:       events.kind_strip1402+		path:       'app'1403+		created_at: -11404+		machine:    'other'1405+	}1406+	backend.put_if_absent(event_key('proj', bad.id), encode_event(bad).bytes())!1407+	assert unapplied_strips(a, mut backend, 'proj')!.len == 01408+}1409+1410+// A resend that fails part way reports what it had already uploaded.1411+fn test_a_failed_resend_reports_what_landed() {1412+	base := scratch('resend_partway')!1413+	defer {1414+		os.rmdir_all(base) or {}1415+	}1416+	remote_dir := os.join_path(base, 'remote')1417+	mut a := open_repo(machine('resend_partway_a')!)!1418+	defer {1419+		a.close()1420+	}1421+	put(a.root, 'one.txt', 'one\n')!1422+	put(a.root, 'two.txt', 'two\n')!1423+	make_save(a, 'two objects')!1424+	hashes := ['one\n', 'two\n'].map(storage.hash_hex(it.bytes()))1425+	mut flaky := FaultyBackend{1426+		inner:      remote.new_folder(remote_dir)!1427+		fail_write: 21428+	}1429+	if _ := resend(a, mut flaky, 'proj', hashes, []) {1430+		assert false, 'the injected failure should have surfaced'1431+	} else {1432+		assert err is Partway1433+		assert (err as Partway).done == 'uploaded 1 object and 0 save records', (err as Partway).done1434+	}13171435 }13181436 13191437 // The local half lands before the remote hears anything, so a remote failure

@@ -84,10 +84,13 @@8484 	}8585 	mut uploaded := Uploaded{}8686 	upload_copies(store, mut backend, repo, hashes, save_ids, mut uploaded) or {87-		return after_failed_upload(store, mut backend, repo, uploaded, true, err)87+		return partway(uploaded.done(), after_failed_upload(store, mut backend, repo, uploaded,88+			err))8889 	}8990 	// As in offer: a strip that landed during the uploads may have missed them.90-	if undo_late_strips(store, mut backend, repo, uploaded, true)! {91+	if undo_late_strips(store, mut backend, repo, uploaded) or {92+		return partway(uploaded.done(), err)93+	} {9194 		return error(strip_not_applied)9295 	}9396 	return uploaded.objects.len + uploaded.saves.len

@@ -10,6 +10,7 @@1010 import json21111 import os1212 import src.events13+import src.fmt1314 import src.remote1415 import src.storage1516 @@ -157,9 +158,20 @@157158 // them. Every step is safe to repeat, and the pending note is cleared only158159 // when all of them have succeeded.159160 pub fn send_strip(store &storage.Store, mut backend remote.Backend, path string) !SendReport {161+	mut report := SendReport{}162+	send_steps(store, mut backend, path, mut report) or {163+		mut done := []string{}164+		if report.records + report.objects > 0 {165+			done << 'rewrote ${fmt.plural(report.records, 'remote record')} and deleted ${fmt.plural(report.objects, 'remote object')} for ${path}'166+		}167+		return partway(done, err)168+	}169+	return report170+}171+172+fn send_steps(store &storage.Store, mut backend remote.Backend, path string, mut report SendReport) ! {160173 	note := read_pending(store, path)!161174 	name := repo_name(store, mut backend)!162-	mut report := SendReport{}163175 	mut upload := OfferReport{164176 		repo: name165177 	}@@ -202,9 +214,24 @@202214 		report.objects++203215 	}204216 	store.remove_state(pending_key(path))!205-	return report206217 }207218 219+// requeue_strip notes every valid recorded strip of `path` again, so that220+// send_strip removes the path from remote records that still hold it.221+pub fn requeue_strip(store &storage.Store, path string) ! {222+	mut note := read_pending(store, path)!223+	recorded := events.strip_events(store)!.filter(events.valid_strip(it) && it.path == path)224+	if recorded.len == 0 {225+		return error('this repo has no recorded strip of ${path}; run koh steal, then koh doctor')226+	}227+	for event in recorded {228+		if event.save_id !in note.saves {229+			note.saves << event.save_id230+		}231+	}232+	store.set_state(pending_key(path), json2.encode(note))!233+}234+208235 // unapplied_strips are the strip events on the remote this repo has not209236 // received yet. offer asks before uploading and again after, because a strip that lands210237 // in between would otherwise leave the stripped path in the bucket.@@ -223,7 +250,7 @@223250 			continue224251 		}225252 		event := decode_event(body.body)!226-		if event.kind == events.kind_strip && !ignored_strip(event) {253+		if events.valid_strip(event) && !ignored_strip(event) {227254 			strips << event228255 		}229256 	}