* ✨ Add storage operation metrics for S3 and buckets
Expose Prometheus metrics for the object storage subsystem.
The S3 backend now attaches an AWS SDK MetricPublisher that counts
API calls, retries and latency per operation and target. The storage
layer counts logical operations and deduplication outcomes per Penpot
bucket, and the assets handlers count served requests per route.
Closes#11676
AI-assisted-by: muse-spark-1.3-contributor
* ✨ Fix storage metrics labels, errors and test gaps
Address the review findings on the storage metrics commit.
Label reads with the object's own backend, count failed asset
serving as errors without swallowing them, and cover the failed
S3 call, S3 asset path and permission-denied branches with tests.
Also share the label helper and reuse the metrics test helper.
Closes#11676
AI-assisted-by: muse-spark-1.3-contributor
* ✨ Harden storage metrics and fill test gaps
Address the second-round review findings on storage metrics.
Unknown backends now fail explicitly and count as errors, exists
stays paired with its dedup outcome, and the thumbnail, missing
storage, expired reads, unknown buckets and write failure paths
are covered by tests. Label coercion goes through the shared
metrics helper.
Closes#11676
AI-assisted-by: muse-spark-1.3-contributor
* ✨ Harden storage metrics accuracy and coverage
Address the full-branch review findings on storage metrics.
Touch and delete emit only on changed rows, reads emit after the
backend fetch, unknown backends fail explicitly, and tempfile
mismatches count as unauthorized. Publisher nil policy, pairing
rules and attempt semantics are documented and covered by tests.
Closes#11676
AI-assisted-by: muse-spark-1.3-contributor
* ✨ Address full-branch review findings on storage metrics
Touch and delete resolve labels from the row, reads stay paired,
failures are covered by tests, and logging, ranges and docs are
tightened. Includes the label helper unit tests and the retries
wording clarification.
Closes#11676
AI-assisted-by: muse-spark-1.3-contributor
* ⚡ Label touch and del metrics from UPDATE RETURNING
The storage metrics change resolved metric labels for touch-object!
and del-object! with an extra SELECT per id-based call. Since app.main
instruments storage unconditionally, every GC collector and binfile
import paid that extra round trip: deleting a team with 10k media
objects doubled the storage_object statements exactly on the paths
that already process the most rows.
touch-object! and del-object! now take only the object id (UUID) and
read the labels from the updated row itself via RETURNING id, backend,
metadata: one statement, no pre-read, and labels that always match the
row actually mutated. del-object! additionally guards on deleted_at
IS NULL, so a repeated delete returns false and emits no metric.
Also from the review of the full branch: extract the duplicated
serve/emit/rethrow block in app.http.assets into one helper; give
penpot_storage_s3_timing explicit histogram buckets up to 60s (the
default cap at 7.5s hid the slow S3 calls the metric exists for);
drop the unused ::target-id config key from the S3 backend and
hardcode the :default target label until per-bucket routing lands.
AI-assisted-by: glm-5.3-flash
* ✨ Harden storage metric recording and definitions
The metric definition schema is now closed and declares every key
the collectors read: buckets, quantiles, max-age and reg. A typo
such as a misspelled ::mdef/buckets used to compile and silently
fall back to the default histogram buckets; it now fails at
startup.
The asset result-label fallback coerced an absent status to 500,
so a future serve path without a status would have counted
successes as errors. The mapping is now explicit and documented:
served below 400, unauthorized for 401/403, not-found for 404,
and error for everything else, including an absent status.
The never-fail try/catch around metric recording existed four
times with drift. One app.metrics/run-safe! helper replaces them:
it no-ops on a nil metrics instance and logs the first failure
per hint at warn level, then at debug, so a broken setup surfaces
once without flooding the log. The S3 publisher keeps its outer
try/catch: it is the SDK MetricPublisher contract boundary.
AI-assisted-by: glm-5.3-flash
* ✨ Make metrics mandatory and run! safe by default
Recording a metric must never change the behavior of the operation
being measured, so `run!` now catches recording failures itself: the
first failure per metric id logs at warn, later ones at debug. This
replaces the `run-safe!` helper, whose four copies had drifted, and
applies the guarantee to every emit site instead of only storage.
The metrics instance precondition is a plain assert, and the collector
lookup stays outside the recording guard, so a missing instance fails
hard even when asserts are disabled. Metrics is therefore no longer
optional: the storage, s3-backend and db-pool schemas require
`::mtx/metrics`, and the assets handler cfg always carries it.
`wrap-publisher` no longer returns nil for a nil instance, and the db
pool wires the prometheus tracker unconditionally.
AI-assisted-by: deepseek-v4.1-flash
* 🐛 Enforce ownership check on tempfile bucket access
The upload-tempfile RPC stores profile-id with tempfile objects, but
objects-handler never verified the requester was the owner. Any
authenticated user who knew the UUID could access the tempfile.
Add ownership check: tempfile bucket now requires the request's
profile-id to match the stored profile-id. Returns 404 on mismatch
(not 403) to avoid leaking object existence.
Legacy tempfiles without stored profile-id remain accessible to any
authenticated user for backward compatibility.
Closes#11269
AI-assisted-by: qwen3.7-plus
* ♻️ Extract tempfile-bucket constant and fix docstring indentation
Extract the 'tempfile' bucket string literal into a named constant
(sto/tempfile-bucket) to prevent typos and make future bucket renames
trivial. Updated 9 occurrences across 7 files.
Also fixed minor docstring indentation inconsistency in authenticated?
function.
AI-assisted-by: qwen3.7-plus
* ♻️ Refactor process-bucket! and authenticated? helpers
Replace case with cond in process-bucket! to properly resolve
sto/tempfile-bucket var from another namespace (case does not
evaluate qualified vars at compile time).
Redefine authenticated? in terms of request-profile-id to remove
duplicated lookup logic.
Closes#11269
AI-assisted-by: mimo-v2.5-pro
PR #11036 added a per-request permission check to the file-media asset
endpoints (/assets/by-file-media-id/:id and the /thumbnail variant) using
bfc/get-file-permissions. Anonymous share-link viewers were then rejected
because they have neither a session nor an access token, the asset URL
carries no share context, and the 2-arg get-file-permissions short-circuits
to nil when profile-id is nil.
Make the asset endpoints share-link aware, mirroring how get-view-only-bundle
already authorizes the same scenario: read the share-id from the query
string, validate it as a UUID, and call the 3-arg perms/get-file-read-permissions
(which chains the existing 2-arg bfc lookup, the bfc share-link fallback,
and the Nitrate org-owner fallback). On the frontend, extend
cf/resolve-file-media with an optional share-id arg and pass it from the
WASM viewer render path using the share-id already present in
[:viewer-local :share-id]. Non-viewer call sites (workspace, clipboard,
code-gen) keep the original URL shape because the new arg defaults to nil.
Closes#11338
AI-assisted-by: minimax-m3
29dbf9ab1 marks non public buckets as attachments, which works on the fs
backend because nginx applies those headers to the internally redirected
response. On the s3 backend the handler answers 307 and the client then
fetches the bytes from the object store, so the header set on the redirect
does not reach the response that carries the object.
Sign the disposition into the presigned url as well, so the object store
returns it. It is only signed when the bucket is not public, so urls for
inline served objects are unchanged.
Also cover the disposition in the handler tests, for the non public buckets
and for the public ones that stay inline.
Co-authored-by: Andrey Antukh <niwi@niwi.nz>
Add authorization check to generic-handler in assets.clj so that
/assets/by-file-media-id/:id and its /thumbnail variant verify the
requesting profile has read access to the parent file. Return 404
(not 403) when access is denied to avoid confirming existence.
Also switch get-file-media-object from db/get to db/get* so that
non-existent media objects return nil instead of raising.
AI-assisted-by: mimo-v2.5-pro
- Add ::setup/props and ::db/pool to :app.http.assets/routes config
so session renewal works correctly for asset requests.
- Add actoken/authz middleware to the assets middleware chain so
access tokens are properly recognized.
- Add authenticated? helper that checks both ::session/profile-id
and ::actoken/profile-id, fixing 401 errors when accessing
protected assets with a valid access token.
- Add comprehensive test suite for assets auth scenarios.
Closes#9677
Signed-off-by: Andrey Antukh <niwi@niwi.nz>
