Roll-forward "Add preliminary support for persisting pre-saved history entries remotely"

This rolls-forward commit 66916c27cb.

backend/server/internal/database/db.go

@@ -152,7 +152,14 @@ func (db *DB) DumpRequestDeleteForUserAndDevice(ctx context.Context, userID, dev
 func (db *DB) ApplyDeletionRequestsToBackend(ctx context.Context, request *shared.DeletionRequest) (int64, error) {
 	tx := db.WithContext(ctx).Where("false")
 	for _, message := range request.Messages.Ids {
-		tx = tx.Or(db.WithContext(ctx).Where("user_id = ? AND device_id = ? AND date = ?", request.UserId, message.DeviceId, message.Date))
+		// Note that this won't do server-side deletion of pre-saved history entries. This is an inherent
+		// limitation of our current DB schema. This is sub-par, since it means that even after deletion, clients
+		// may still receive deleted history entries. But, a well-behaved client will immediately delete
+		// these (never writing them to disk) and mark them as received, so this won't happen again.
+		//
+		// TODO: This could be improved upon if we added a HistoryEntry.EntryId field, backfilled it, added
+		// it to EncHistoryEntry, and then used that as a key for deletion.
+		tx = tx.Or(db.WithContext(ctx).Where("user_id = ? AND device_id = ? AND date = ?", request.UserId, message.DeviceId, message.EndTime))
 	}
 	result := tx.Delete(&shared.EncHistoryEntry{})
 	if tx.Error != nil {
@@ -226,7 +233,7 @@ func (db *DB) Clean(ctx context.Context) error {
 	if r.Error != nil {
 		return r.Error
 	}
-	r = db.WithContext(ctx).Exec("DELETE FROM deletion_requests WHERE read_count > 100")
+	r = db.WithContext(ctx).Exec("DELETE FROM deletion_requests WHERE read_count > 1000")
 	if r.Error != nil {
 		return r.Error
 	}

backend/server/internal/server/server_test.go

@@ -411,7 +411,7 @@ func TestDeletionRequests(t *testing.T) {
 		UserId:   data.UserId("dkey"),
 		SendTime: delReqTime,
 		Messages: shared.MessageIdentifiers{Ids: []shared.MessageIdentifier{
-			{DeviceId: devId1, Date: entry1.EndTime},
+			{DeviceId: devId1, EndTime: entry1.EndTime},
 		}},
 	}
 	reqBody, err = json.Marshal(delReq)
@@ -507,7 +507,7 @@ func TestDeletionRequests(t *testing.T) {
 		SendTime:            delReqTime,
 		ReadCount:           1,
 		Messages: shared.MessageIdentifiers{Ids: []shared.MessageIdentifier{
-			{DeviceId: devId1, Date: entry1.EndTime},
+			{DeviceId: devId1, EndTime: entry1.EndTime},
 		}},
 	}
 	if diff := deep.Equal(*deletionRequest, expected); diff != nil {

client/client_test.go

@@ -2364,15 +2364,24 @@ func testPresaving(t *testing.T, tester shellTester) {
 	out = tester.RunInteractiveShell(t, ` hishtory query sleep 13371337 -export -tquery`)
 	testutils.CompareGoldens(t, out, "testPresaving-query")
 
-	// And the same for tquery
-	// out = captureTerminalOutputWithComplexCommands(t, tester,
-	// 	[]TmuxCommand{
-	// 		{Keys: "hishtory SPACE tquery ENTER", ExtraDelay: 2.0},
-	// 		{Keys: "sleep SPACE 13371337 SPACE -export SPACE -tquery"}})
-	// out = strings.TrimSpace(strings.Split(out, "hishtory tquery")[1])
-	// testutils.CompareGoldens(t, out, "testPresaving-tquery")
-	//
-	// TODO: Debug why ^ is failing with flaky differences on Github Actions, see https://pastebin.com/BUa1btnh
+	// Create a new device, and confirm it shows up there too
+	restoreDevice1 := testutils.BackupAndRestoreWithId(t, "device1")
+	installHishtory(t, tester, userSecret)
+	tester.RunInteractiveShell(t, ` hishtory config-set displayed-columns CWD Runtime Command`)
+	out = tester.RunInteractiveShell(t, ` hishtory query sleep 13371337 -export -tquery`)
+	testutils.CompareGoldens(t, out, "testPresaving-query")
+
+	// And then redact it from device2
+	tester.RunInteractiveShell(t, ` HISHTORY_REDACT_FORCE=true hishtory redact sleep 13371337`)
+
+	// And confirm it was redacted
+	out = tester.RunInteractiveShell(t, ` hishtory export sleep -export`)
+	require.Equal(t, "", out)
+
+	// Then go back to device1 and confirm it was redacted there too
+	restoreDevice1()
+	out = tester.RunInteractiveShell(t, ` hishtory export sleep -export`)
+	require.Equal(t, "", out)
 }
 
 func testUninstall(t *testing.T, tester shellTester) {

client/cmd/redact.go

@@ -86,7 +86,9 @@ func deleteOnRemoteInstances(ctx context.Context, historyEntries []*data.History
 	deletionRequest.UserId = data.UserId(config.UserSecret)
 
 	for _, entry := range historyEntries {
-		deletionRequest.Messages.Ids = append(deletionRequest.Messages.Ids, shared.MessageIdentifier{Date: entry.EndTime, DeviceId: entry.DeviceId})
+		deletionRequest.Messages.Ids = append(deletionRequest.Messages.Ids,
+			shared.MessageIdentifier{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId},
+		)
 	}
 	return lib.SendDeletionRequest(deletionRequest)
 }

client/cmd/saveHistoryEntry.go

@@ -55,6 +55,7 @@ func maybeUploadSkippedHistoryEntries(ctx context.Context) error {
 
 	// Upload the missing entries
 	db := hctx.GetDb(ctx)
+	// TODO: There is a bug here because MissedUploadTimestamp is going to be a second or two after the history entry that needs to be uploaded
 	query := fmt.Sprintf("after:%s", time.Unix(config.MissedUploadTimestamp, 0).Format("2006-01-02"))
 	entries, err := lib.Search(ctx, db, query, 0)
 	if err != nil {
@@ -81,6 +82,21 @@ func maybeUploadSkippedHistoryEntries(ctx context.Context) error {
 	return nil
 }
 
+func handlePotentialUploadFailure(err error, config *hctx.ClientConfig) {
+	if err != nil {
+		if lib.IsOfflineError(err) {
+			hctx.GetLogger().Infof("Failed to remotely persist hishtory entry because we failed to connect to the remote server! This is likely because the device is offline, but also could be because the remote server is having reliability issues. Original error: %v", err)
+			if !config.HaveMissedUploads {
+				config.HaveMissedUploads = true
+				config.MissedUploadTimestamp = time.Now().Unix()
+				lib.CheckFatalError(hctx.SetConfig(*config))
+			}
+		} else {
+			lib.CheckFatalError(err)
+		}
+	}
+}
+
 func presaveHistoryEntry(ctx context.Context) {
 	config := hctx.GetConf(ctx)
 	if !config.IsEnabled {
@@ -119,12 +135,13 @@ func presaveHistoryEntry(ctx context.Context) {
 	lib.CheckFatalError(err)
 	db.Commit()
 
-	// Note that we aren't persisting these half-entries remotely,
-	// since they should be updated with the rest of the information very soon. If they
-	// are never updated (e.g. due to a long-running command that never finishes), then
-	// they will only be available on this device. That isn't perfect since it means
-	// history entries can get out of sync, but it is probably good enough.
-	// TODO: Consider improving this
+	// And persist it remotely
+	if !config.IsOffline {
+		jsonValue, err := lib.EncryptAndMarshal(config, []*data.HistoryEntry{entry})
+		lib.CheckFatalError(err)
+		_, err = lib.ApiPost("/api/v1/submit?source_device_id="+config.DeviceId, "application/json", jsonValue)
+		handlePotentialUploadFailure(err, &config)
+	}
 }
 
 func saveHistoryEntry(ctx context.Context) {
@@ -175,6 +192,7 @@ func saveHistoryEntry(ctx context.Context) {
 		jsonValue, err := lib.EncryptAndMarshal(config, []*data.HistoryEntry{entry})
 		lib.CheckFatalError(err)
 		w, err := lib.ApiPost("/api/v1/submit?source_device_id="+config.DeviceId, "application/json", jsonValue)
+		handlePotentialUploadFailure(err, &config)
 		if err == nil {
 			submitResponse := shared.SubmitResponse{}
 			err := json.Unmarshal(w, &submitResponse)
@@ -183,17 +201,6 @@ func saveHistoryEntry(ctx context.Context) {
 			}
 			shouldCheckForDeletionRequests = submitResponse.HaveDeletionRequests
 			shouldCheckForDumpRequests = submitResponse.HaveDumpRequests
-		} else {
-			if lib.IsOfflineError(err) {
-				hctx.GetLogger().Infof("Failed to remotely persist hishtory entry because we failed to connect to the remote server! This is likely because the device is offline, but also could be because the remote server is having reliability issues. Original error: %v", err)
-				if !config.HaveMissedUploads {
-					config.HaveMissedUploads = true
-					config.MissedUploadTimestamp = time.Now().Unix()
-					lib.CheckFatalError(hctx.SetConfig(config))
-				}
-			} else {
-				lib.CheckFatalError(err)
-			}
 		}
 	}
 

client/lib/lib.go

@@ -600,7 +600,10 @@ func ProcessDeletionRequests(ctx context.Context) error {
 	db := hctx.GetDb(ctx)
 	for _, request := range deletionRequests {
 		for _, entry := range request.Messages.Ids {
-			res := db.Where("device_id = ? AND end_time = ?", entry.DeviceId, entry.Date).Delete(&data.HistoryEntry{})
+			// Note that entry.StartTime is not always present (for legacy reasons) and entry.EndTime is also
+			// not always present (for pre-saved entries). So we just check that one of them matches.
+			tx := db.Where("device_id = ? AND (start_time = ? OR end_time = ?)", entry.DeviceId, entry.StartTime, entry.EndTime)
+			res := tx.Delete(&data.HistoryEntry{})
 			if res.Error != nil {
 				return fmt.Errorf("DB error: %w", res.Error)
 			}

client/tui/tui.go

@@ -601,7 +601,9 @@ func deleteHistoryEntry(ctx context.Context, entry data.HistoryEntry) error {
 		UserId:   data.UserId(hctx.GetConf(ctx).UserSecret),
 		SendTime: time.Now(),
 	}
-	dr.Messages.Ids = append(dr.Messages.Ids, shared.MessageIdentifier{Date: entry.EndTime, DeviceId: entry.DeviceId})
+	dr.Messages.Ids = append(dr.Messages.Ids,
+		shared.MessageIdentifier{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId},
+	)
 	return lib.SendDeletionRequest(dr)
 }
 

shared/data.go

@@ -9,13 +9,14 @@ import (
 
 // Represents an encrypted history entry
 type EncHistoryEntry struct {
-	EncryptedData []byte    `json:"enc_data"`
-	Nonce         []byte    `json:"nonce"`
-	DeviceId      string    `json:"device_id"`
-	UserId        string    `json:"user_id"`
-	Date          time.Time `json:"time"`
-	EncryptedId   string    `json:"id"`
-	ReadCount     int       `json:"read_count"`
+	EncryptedData []byte `json:"enc_data"`
+	Nonce         []byte `json:"nonce"`
+	DeviceId      string `json:"device_id"`
+	UserId        string `json:"user_id"`
+	// Note that EncHistoryEntry.Date == HistoryEntry.EndTime
+	Date        time.Time `json:"time"`
+	EncryptedId string    `json:"id"`
+	ReadCount   int       `json:"read_count"`
 }
 
 /*
@@ -87,8 +88,11 @@ type MessageIdentifiers struct {
 type MessageIdentifier struct {
 	// The device that the entry was recorded on (NOT the device where it is stored/requesting deletion)
 	DeviceId string `json:"device_id"`
-	// The timestamp when the message finished running
-	Date time.Time `json:"date"`
+	// The timestamp when the command finished running. Serialized as "date" for legacy compatibility.
+	EndTime time.Time `json:"date"`
+	// The timestamp when the command started running.
+	// Note this field was added as part of supporting pre-saving commands, so older clients do not set this field
+	StartTime time.Time `json:"start_time"`
 }
 
 func (m *MessageIdentifiers) Scan(value interface{}) error {