Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions internal/tigerfs/db/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -1191,6 +1191,37 @@ func (c *Client) QueryLogEntry(ctx context.Context, schema, logTable, logID stri
return &f, nil
}

// emptyVersionIDToNull converts an empty version_id captured during
// ExecuteUndoTransaction Step 1/2 into a SQL NULL (returned as nil
// interface{}) for the Step 3 INSERT, while logging a warning so the
// underlying workspace inconsistency is surfaced to operators.
//
// The inline captures in Steps 1 and 2 only fail to populate the
// captured map when the BEFORE archive trigger didn't write a history
// row for the mutation -- typically because the trigger is missing or
// disabled on a malformed workspace. Recording NULL on the resulting
// undo log row keeps the transaction from failing with a confusing
// "invalid input syntax for type uuid" PG error (the version_id column
// can't accept the empty string literal). NULL is already a legitimate
// value for the column (create-type log entries store NULL too), and
// downstream undo-of-undo dispatch handles empty/NULL version_id
// cleanly by returning a clear "no version_id" error.
//
// branch is "delete" or "restore" -- it labels which Step 3 loop hit
// the empty capture, helping correlate the warning to the affected
// path. fileID is the source row that triggered the empty capture.
func emptyVersionIDToNull(versionID, fileID, branch string) interface{} {
if versionID != "" {
return versionID
}
logging.Warn("undo log entry has no version_id; archive trigger may not be firing",
zap.String("file_id", fileID),
zap.String("branch", branch),
zap.String("hint", "verify the archive BEFORE trigger exists on the source table; "+
"this undo log entry's version_id was recorded as NULL and the entry will not be reversible"))
return nil
}

// ExecuteUndoTransaction executes a batch of undo operations atomically.
// Within a single transaction: deletes created rows, upserts from history,
// and inserts undo log entries. BEFORE triggers fire on each restore,
Expand Down Expand Up @@ -1390,27 +1421,37 @@ func (c *Client) ExecuteUndoTransaction(ctx context.Context, params *UndoTransac
if i < len(params.DeleteFilenames) {
filename = params.DeleteFilenames[i]
}
versionIDVal := capturedDeleteVIDs[fileID]
// Pass nil (-> SQL NULL) when the inline capture in Step 1 produced no
// version_id. The Step 1 capture only fails to populate the map when
// the BEFORE-DELETE archive trigger didn't write a history row -- an
// invariant violation in a properly-configured workspace, but possible
// if the trigger is missing or disabled. Recording NULL instead of an
// empty string keeps the transaction from failing with a cryptic
// "invalid input syntax for type uuid" and lets the undo complete;
// the warning below surfaces the underlying workspace inconsistency.
// See ExecuteUndoTransaction's Step 1 comment for the capture invariant.
versionIDArg := emptyVersionIDToNull(capturedDeleteVIDs[fileID], fileID, "delete")
if _, err := tx.Exec(ctx,
fmt.Sprintf(
`INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s) VALUES (uuidv7(), $1, 'undo', $2, $3, $4, $5)`,
logTable, qi("log_id"), qi("file_id"), qi("type"), qi("user_id"), qi("filename"), qi("version_id"), qi("description"),
),
fileID, params.UserID, filename, versionIDVal, params.Description,
fileID, params.UserID, filename, versionIDArg, params.Description,
); err != nil {
return fmt.Errorf("failed to insert undo log entry for delete: %w", err)
}
}
for i, fileID := range params.RestoreFileIDs {
versionIDVal := capturedRestoreVIDs[fileID]
// Same nil-conversion + warning as the DELETE branch above.
versionIDArg := emptyVersionIDToNull(capturedRestoreVIDs[fileID], fileID, "restore")
if _, err := tx.Exec(ctx,
fmt.Sprintf(
`INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s) VALUES (uuidv7(), $1, 'undo', $2, $3, $4, $5)`,
logTable,
qi("log_id"), qi("file_id"), qi("type"), qi("user_id"),
qi("filename"), qi("version_id"), qi("description"),
),
fileID, params.UserID, params.RestoreFilenames[i], versionIDVal, params.Description,
fileID, params.UserID, params.RestoreFilenames[i], versionIDArg, params.Description,
); err != nil {
return fmt.Errorf("failed to insert undo log entry for restore: %w", err)
}
Expand Down
25 changes: 20 additions & 5 deletions internal/tigerfs/db/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2383,11 +2383,26 @@ func TestExecuteUndoTransaction_DefersFKMultiLevel(t *testing.T) {
}
}

// TestExecuteUndoTransaction_DeleteOnlyUndo guards the empty-RestoreFileIDs
// branch of the post-undo modified_at bump. When a user undoes a single
// 'create' op, the only work is to DELETE the created row from source -- no
// rows are restored. The mtime-bump SQL uses ANY($1::uuid[]) and would
// error on an empty array if the branch wasn't gated.
// TestExecuteUndoTransaction_DeleteOnlyUndo guards two distinct edge cases
// of a DELETE-only undo (single 'create' op being reversed):
//
// 1. The empty-RestoreFileIDs branch of the post-undo modified_at bump.
// That SQL uses ANY($1::uuid[]) and would error on an empty array if
// the branch wasn't gated.
//
// 2. The defensive NULL-version_id path in Step 3. The fixture deliberately
// doesn't install the archive BEFORE-DELETE trigger (setupUndoTestEnv
// keeps the schema trigger-free so other tests can seed history rows
// directly without interference), which means Step 1's DELETE fires no
// trigger and writes no history row. The inline version_id capture
// finds nothing. Step 3 must record the resulting undo log entry with
// version_id = NULL rather than failing with PG's "invalid input
// syntax for type uuid" on the empty string. In a properly-configured
// production workspace the archive trigger always fires and this
// defensive path is dead code; the test holds it down regardless so a
// future regression doesn't reintroduce the cryptic-error failure
// mode. emptyVersionIDToNull (in query.go) handles the conversion and
// emits a warning identifying the file_id and the branch.
func TestExecuteUndoTransaction_DeleteOnlyUndo(t *testing.T) {
env, cleanup := setupUndoTestEnv(t)
defer cleanup()
Expand Down