photoprism / photoprism

AI-Powered Photos App for the Decentralized Web 🌈💎✨
https://www.photoprism.app
Other
34.68k stars 1.91k forks source link

API: Some updates fail silently in case of database errors #4504

Closed keif888 closed 2 days ago

keif888 commented 1 week ago

1. What is not working as documented?

When issuing a call to the update photo api with location (latitude/longitude) changes, and the database has a lock timeout or a deadlock occur, the api fails silently and returns a http 200 response, but does NOT update the location information.

api: PUT call to /api/v1/photos/[photoid] body: {"Lat": 20.3456, "Lng":20,3456, "PlaceSrc": "manual"}

2. How can we reproduce it?

The steps below use the PhotoPrism app and the MariaDB client (or any other db client to connect to MariaDB) to reproduce the issue. I used the app in the reproduction as it uses the /api/v1/photos under the covers.

Steps to reproduce the behavior:

  1. choose a photo that you will update, and note the labels that are applied to it
  2. choose a label that you will use to simulate the issue, and find it's id. Replace < value > with the label name you chose.
    select id, label_name, photo_count from photoprism.labels where label_name = '<value>';
  3. edit the photo
  4. change the longitude and latitude, but do NOT press APPLY or DONE.
  5. issue the following sql commands replacing the < id > with the number of the label that you chose above:
    set autocommit = 0;
    begin work;
    update photoprism.labels
    set photo_count = 2
    where id = <id>;
  6. press DONE in the photoprism app
  7. wait for the lock wait timeout exceeded that will occur (this took 4 minutes in my testing)
  8. the photoprism app should now have returned to the library, without any errors shown
  9. open the selected photo, and note that the longitude and latitude have not been saved
  10. rollback the database update
    rollback work;

3. What behavior do you expect?

The api call should return a http 500 response if the update is unable to be applied. The app should have a pop up message to show that the update was not done.

4. What could be the cause of your problem?

photoprism/internal/entity/entity_update.go is not returning the error that is being generated by an update, but the lack of an error from an earlier database call (values, keys, err := ModelValues(m, keyNames...)). https://github.com/photoprism/photoprism/blob/98b0dc5e07ac80fe83b439478fe767f4760bc01a/internal/entity/entity_update.go#L32

    // Extract interface slice with all values including zero.
    values, keys, err := ModelValues(m, keyNames...)

    // Has keys and values?
    if err != nil {
        return err
    } else if len(keys) != len(keyNames) {
        return fmt.Errorf("record keys missing")
    }

    // Update values.
    result := db.Model(m).Updates(values)

    // Successful?
    if result.Error != nil {
        return err  // <<--------  This is wrong!!!
    } else if result.RowsAffected > 1 {
        log.Debugf("entity: updated statement affected more than one record - you may have found a bug")
        return nil

It should be:

    // Update values.
    result := db.Model(m).Updates(values)

    // Successful?
    if result.Error != nil {
        return result.Error  // <--- This needs to be changed as shown
    } else if result.RowsAffected > 1 {
        log.Debugf("entity: updated statement affected more than one record - you may have found a bug")
        return nil

5. Can you provide us with example files for testing, error logs, or screenshots?

Log files showing a deadlock message, and the successful result message:

photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="place: gb:u7WNUrVi23K5 not found"
photoprism-1  | time="2024-09-04T12:28:12Z" level=info msg="cell: added place gb:u7WNUrVi23K5 [1.290580231s]"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="cell: added s2:487604ce4af4 [1.292516235s]"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="photo: changing location of 2024/09/20240904_120425_406A1B87 from s2:465253379adc to s2:487604ce4af4"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="photo: 2024/09/20240904_120425_406A1B87 title based on label Memorial"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="photo: 2024/09/20240904_120425_406A1B87 has new title 'Memorial / Westminster / 2024' [1.253884ms]"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="search: updated index by photo id [1.135424ms]"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="index: updating counts"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="counts: updated 1 place [1.633586ms]"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="counts: updated 0 subjects [644.176µs]"
photoprism-1  | time="2024-09-04T12:28:12Z" level=warning msg="index: Error 1213 (40001): Deadlock found when trying to get lock; try restarting transaction while updating label counts (update counts)"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="photos: found 1 result for uid:psjaew5oxefktsbe dist:2.000000 merged:true [1.219616ms]"
photoprism-1  | time="2024-09-04T12:28:12Z" level=info msg="changes successfully saved"
photoprism-1  | time="2024-09-04T12:28:12Z" level=info msg="photo: updated sidecar file 2024/09/20240904_120425_406A1B87.yml"
photoprism-1  | time="2024-09-04T12:28:12Z" level=debug msg="server: PUT /api/v1/photos/psjaew5oxefktsbe (200) [1.40565443s]"

You can use any photo for testing, so long as it has labels against it. The reproduce steps above produce a lock timeout instead of a deadlock, but the code path followed within the PhotoPrism application is the same.

6. Which software versions do you use?

(a) PhotoPrism Architecture & Build Number: AMD64 - Build 240906-7792abf9a

(b) Database Type & Version: MariaDB 11.5.2

(c) Operating System Types & Versions: Linux Debian 12.7

(d) Browser Types & Versions: Chrome 128.0.6613.119 64 bit

(e) Ad Blockers, Browser Plugins, and/or Firewall Software - None, clean fresh install of everything. Extension Geolocation Plugin for PhotoPrism is installed.

7. On what kind of device is PhotoPrism installed?

This is especially important if you are reporting a performance, import, or indexing issue. You can skip this if you're reporting a problem you found in our public demo, or if it's a completely unrelated issue, such as incorrect page layout.

(a) Device / Processor Type: AMD Ryzen 7 5700X in VirtualBox (4 cores assigned) or Intel Core i3 540 in NAS

(b) Physical Memory & Swap Space in GB 8Gb ram, 16Gb swap for both

(c) Storage Type: HDD, SSD, RAID, USB, Network Storage,... SSD for Virtual Box HDD for NAS

(d) Anything else that might be helpful to know? I will try and put a Pull request to fix this issue, as I've already tested the code fix mentioned above.

8. Do you use a Reverse Proxy, Firewall, VPN, or CDN?

No

lastzero commented 6 days ago

@keif888 Thank you for your detailed report and fix! I have updated the entity.Update() function based on this and would appreciate it if you could test these changes, see https://github.com/photoprism/photoprism/commit/be00bcf0b34d037d77360efc3e4a2c924fea1ebf.