[s3inbox] port v3.1.0 hotfix for concurrent map read / write to main

[KarlG-nbis]

We had issue in PRD with concurrent map read / writes in the s3inbox with concurrent uploads.

fatal error: concurrent map writes

goroutine 13331 [running]:
internal/runtime/maps.fatal({0xb71015?, 0xab27a0?})
        /usr/local/go/src/runtime/panic.go:1058 +0x18
internal/runtime/maps.(*Map).Delete(0xc0004dd860, 0xab27a0, 0xc0005fb958)
        /usr/local/go/src/internal/runtime/maps/map.go:672 +0x10f
main.(*Proxy).allowedResponse(0xc0000a8180, {0xc92558, 0xc0034001c0}, 0xc0025f8000, {0xc99558, 0xc003e4a850})
        /go/cmd/s3inbox/proxy.go:254 +0x10f6
main.(*Proxy).ServeHTTP(0xc0000a8180, {0xc92558, 0xc0034001c0}, 0xc0025f8000)
        /go/cmd/s3inbox/proxy.go:109 +0x17b
github.com/gorilla/mux.(*Router).ServeHTTP(0xc0000a80c0, {0xc92558, 0xc0034001c0}, 0xc00330bb80)
        /root/go/pkg/mod/github.com/gorilla/mux@v1.8.1/mux.go:212 +0x1e2
net/http.serverHandler.ServeHTTP({0xc8e690?}, {0xc92558?, 0xc0034001c0?}, 0x6?)
        /usr/local/go/src/net/http/server.go:3301 +0x8e
net/http.(*conn).serve(0xc004859c20, {0xc931c8, 0xc0004ddf50})
        /usr/local/go/src/net/http/server.go:2102 +0x625
created by net/http.(*Server).Serve in goroutine 1
        /usr/local/go/src/net/http/server.go:3454 +0x485

Hotfix was prepared based on v3.1.0 so we could fix it in PRD: 822354f but we also need to port this to main which is the aim of this task.

In addition the sync.RWMutex there are additional improvements to be made, eg:

• resuse fileID read from cache instead of reading each time
  • better performance since each call to .read() will trigger a RLock() and RUnlock().
  • in a highly concurrent environment, this avoids the problems that each call may return different results, sometimes might be empty if another process deleted the entry, etc

[jhagberg]

The RWMutex fix in 822354f correctly prevents the concurrent map writes crash, but there are remaining TOCTOU (Time-of-Check-Time-of-Use) races in allowedResponse where p.fileIDs.read() is called multiple times for the same key without holding the lock across the block.

For example, lines 269-284:

p.storeObjectSizeInDB(rawFilepath, p.fileIDs.read(fileIdentifier))
p.database.UpdateFileEventLog(p.fileIDs.read(fileIdentifier), ...)
p.fileIDs.delete(fileIdentifier)

Between each .read() the lock is released, so another goroutine could .delete() the entry, causing subsequent reads to return "".

Suggested fix: Read once into a local variable at the start of each block and reuse it. This avoids both the repeated locking overhead and the inconsistency risk.

currentFileID := p.fileIDs.read(fileIdentifier)
if currentFileID == "" {
    // ...
}

This applies to the blocks starting around lines 200, 248, and 257.

[jbygdell]

There is one more dimension to this.

With multiple pods running there is a possibility that a multipart file upload will start on one pod and finish on another. Leading to the delete call to be left out on one of the pods and that will result in memory ballooning (albeit a small one).

This also means that calls like sendMessageOnOverwrite will be triggered once for each pod that is running since all calls will be routed round-robin between the pods.
Unless the client is set to make use of cookies so the calls can be routed to the same pod. (not all clients do this)

We could use the PostgresDB as a KV store instead of memory at a slight cost of throughput, or go with something more complex like an in memory replicated RaftDB.