protomaps / go-pmtiles

Single-file executable tool for working with PMTiles archives
https://docs.protomaps.com/pmtiles/cli
BSD 3-Clause "New" or "Revised" License
359 stars 49 forks source link

Azure Reloading pmtiles header on file change #176

Closed akhenakh closed 1 week ago

akhenakh commented 1 month ago

Before working on a patch I wanted to have your opinion.

In the context of the Caddy pmtiles plugins: it's using the modification time of the file to compute the etag (at least in the file implementation but same for s3 https://github.com/protomaps/go-pmtiles/blob/main/pmtiles/bucket.go#L121 ), but it seems nothing is reloading the header, then the tile requests are pointing to wrong offsets returning bogus data.

Since we can detect the file changed we could reload and set the proper offsets?

Note while following the code I'm not sure the cache is working correctly if you host multiple pmtiles since the key is just formed on the zxy without the path?

Thanks!

bdon commented 1 month ago

nothing is reloading the header

The JavaScript client should detect the new etag. https://github.com/protomaps/PMTiles/blob/main/js/index.ts#L1019

What client are you using where this isn't working?

Note while following the code I'm not sure the cache is working correctly if you host multiple pmtiles since the key is just formed on the zxy without the path?

The key is part of the cache: https://github.com/protomaps/go-pmtiles/blob/main/pmtiles/server.go#L19

akhenakh commented 1 month ago

Sorry for the lack of context, I'm using the xyz tiles served by caddy and the pmtiles plugin, in that context the js client does not have access to the pmtiles headers.

Only the server can detect the file modification time changed (it does) but it is not reloading the correct seek position, so it returns back garbage.

bdon commented 1 month ago

OK, that sounds like a bug. Just to be sure, what OS are you using?

akhenakh commented 1 month ago

OS is Linux, it has been observed and reproduced while using the Azure blobstorage from the gcloud library, but from the code I think it would happen on any sources.

bdon commented 1 month ago

So are you observing this only on serving archives stored on Azure Blob, or also on the filesystem?

@msbarry may have mentioned in the PR that Azure etags aren't supported yet: https://github.com/protomaps/go-pmtiles/pull/130

akhenakh commented 1 month ago

thanks! This is a serious lead, I tested it again with the local file adapter and you are right it worked, the etag changed and the correct file is returned.

bdon commented 1 month ago

Basically this line is responsible for turning the abstract Bucket into a provider-specific one. Only the logic for S3 is implemented correctly right now: https://github.com/protomaps/go-pmtiles/blob/main/pmtiles/bucket.go#L216

So we would need to check if the low-lying azblob library supports If-Match, and if so populate it there.

akhenakh commented 1 month ago

I've tested this PR on the local azure blob emulator, will do more testing against the real storage and let you know.

bdon commented 1 week ago

Cut a release in https://github.com/protomaps/go-pmtiles/releases/tag/v1.21.0