feat(sequences): add detection sampling, plus S3 bucket and presigned-URL caching

[Acruve15]

Closes #660.

The issue asks for detection sampling so the player can load a sequence without pulling every frame, and names two causes: API response time and client-side caching. Investigating GET /sequences/{id}/detections turned up three further problems, two of which hit harder than sampling itself, so this PR does three things, with the fourth split out into #664.

What changed

1. Sampling (the issue's ask). sampling=N keeps one detection in N, pushed into SQL via row_number() rather than slicing a full fetch in Python (which would keep the scan and the serialization cost and only shrink the payload). sampling=1 delegates to the existing path, so default behaviour is unchanged.

2. S3Bucket instance caching. get_bucket() built a fresh instance per call and S3Bucket.__init__ does a blocking head_bucket, so every request paid one synchronous S3 round-trip on the event loop.

3. Stable presigned URLs + Cache-Control. boto3 stamps the current clock into every signature, so presigning the same key twice returns two different strings. The browser keys its cache on the full URL, so the player re-downloaded every frame on every poll even when the image had not changed. This is the client-side-caching half of the issue, and sampling alone does nothing for it.

Performance, before vs after

The index this depends on lives in #664

The composite index on detections(sequence_id, created_at) was originally part of this PR, but it turned out to be exactly one of the three indexes proposed in #663, so it moved to #664 to keep a single owner. The DB numbers below assume #664 has landed. Without it, the sequence read stays on a sequential scan and this PR's gain is limited to the S3 items.

Measured there: 34.8 ms to 0.42 ms at 2M detections (roughly production today), and 63.6 ms to 1.1 ms at 5M.

Per request, 100 frames returned, 5M-row table

Component	Before	After
DB query (needs #664)	63 to 84 ms (seq scan)	1.1 ms (index scan)
head_bucket (once per request)	10.1 ms	0 (cached instance)
Presigning 100 URLs	7.5 ms	~0 (cached, warm)
Framework + JSON	~3 ms	~3 ms
Total	~85 ms	~4 ms (≈20x)

On top of that, sampling changes the shape of the work

Loading a 1000-frame timeline previously meant 10 requests (the old limit ceiling was 100), each paying the ~85 ms above. It is now one request at sampling=10, and the browser fetches 100 images instead of 1000.

Worth stating plainly: most of that last factor is returning a tenth of the data by design, not the same work done faster. The honest split is roughly 20x from the fixes (of which the DB share needs #664) and 10x fewer bytes and images from sampling.

The browser-cache win is the one number missing here. Every poll after the first goes from re-downloading N images to N cache hits, which is probably the largest perceived gain for the player, but quantifying it needs the frontend rather than the API.

How this was measured, and two corrections

Numbers come from a seeded 5M-row database (50k sequences) on the dev compose stack. Before/after uses the same data, forcing the pre-change plan with enable_indexscan=off rather than dropping and rebuilding the index, so the two measurements are directly comparable.

Two measurement traps worth recording, since both changed the answer materially:

• Heap clustering, which moved the index numbers by nearly 3x. Written up in perf(db): add hot-path indexes for sequence matching, latest-bbox and shared frame lookups #664, where that measurement now lives.
• A 64KB loopback artifact. End-to-end HTTP timings showed 100 frames at 47 ms but 500 frames at 20 ms, which is absurd. It survived reordering and isolation. Sweeping limit located a ~45 ms penalty that vanished between 60.9KB and 65.0KB of response body: the 64KB socket buffer, i.e. a 40 ms delayed-ACK stall on loopback inside the container. It affects before and after equally and is not a property of the API, but it makes absolute sub-64KB latencies from that harness meaningless, so the table above is built from measured components instead. Above 64KB timings go cleanly linear at ~0.03 ms per frame.

API compatibility

No breaking change. Only one route's signature moved, and only additively:

• sampling added, default 1 = exactly the previous behaviour
• limit ceiling widened 100 to 500, so any previously valid request stays valid
• two description strings reworded

The response model is untouched. No routes were added, removed, or renamed, and auth is unchanged. The one behavioural change reaching every route that returns a presigned URL is that URLs are now stable within a window instead of unique per request, which is the point of item 3.

For pyro-platform: nothing is required, and items 2 and 3 speed up the existing call with no frontend work. To get the sampling win it needs to pass sampling, and also limit: getDetectionsBySequence currently sends only desc, so it receives the API default of 10 detections.

sampling and limit are independent, which is the easy thing to get wrong: ?sampling=10 alone, with the desc=true and limit=10 defaults, returns the 10 most recent sampled frames, not a spread across the sequence. Spanning the sequence in one call needs limit >= ceil(detections_count / sampling), and since limit caps at 500, sampling >= detections_count / 500. detections_count is already on the sequence object, so no extra endpoint is needed. Both the endpoint description and the client docstring now spell this out.

A 20/20/20 player load on a 1000-detection sequence, verified against the endpoint rather than derived on paper:

?desc=false&limit=20                        -> frames 1..20
?desc=false&sampling=48&offset=1&limit=20   -> frames 49..961, step 48
?desc=true&limit=20                         -> frames 981..1000 (reverse client-side)

60 distinct frames, no overlap between the three calls. General form for M middle frames: sampling = floor((N - 40) / M), offset = ceil(20 / sampling), limit = M. Credit to @fe51 for the recipe.

Interaction with #624 (now merged)

Rebased onto main after #624 landed. Two points a reviewer should know:

• Continuity rows are sampled too. feat(detections): accept empty bboxes and keep sequence frames continuous #624 made GET /sequences/{id}/detections return continuity rows (frames attached with bbox="[]" where the object was not detected), and its own route description tells clients to filter on bbox if they only want real detections. Sampling thins that combined set rather than treating continuity rows specially, which keeps it consistent with the documented endpoint behaviour and with detections_count, which feat(detections): accept empty bboxes and keep sequence frames continuous #624 also made include them.
• crud_detection.py conflicted and was resolved by hand. feat(detections): accept empty bboxes and keep sequence frames continuous #624 added get_latest_with_bbox, which needs sqlmodel's select (a single-entity SelectOfScalar is what makes session.exec return a Detection rather than a Row). The sampled query needs sqlalchemy's select for its two-entity numbering subquery. Both now coexist: select stays sqlmodel's, as feat(detections): accept empty bboxes and keep sequence frames continuous #624 wrote it, and sqlalchemy's is imported as select_sa, so no line of feat(detections): accept empty bboxes and keep sequence frames continuous #624's code changed.

Semantics worth reviewing

These are the decisions a reviewer should push on:

Decision	Choice	Why
Interval vs target count	Interval (sampling=N)	The issue itself notes interval is simpler and more predictable
Row numbering direction	Always ascending on created_at	The sampled frame set is then identical for desc=true and desc=false; desc only flips output order. It also means an appended detection cannot renumber earlier rows, so the player keeps hitting the same frames and the same cached URLs across polls
Which rows are kept	(rn - 1) % sampling == 0	Keeps detection #1, so any non-empty sequence returns at least one row
offset	Pages the sampled set, not raw rows	Predictable for the client
limit ceiling	100 to 500	The issue's own example (1000 detections at sampling=10) saturated 100 exactly, so sampling=5 would have been silently truncated

Deliberately not id % N: detection ids are global rather than per-sequence, so the stride would be non-uniform and small sequences could return zero rows.

Two known limitations: limit cannot push down through row_number(), so the scan is proportional to sequence length regardless of limit (inherent to interval sampling, and why the index matters); and sampling is uniform in created_at, not recorded_at, so out-of-order uploads make it non-uniform in capture time.

Verification

• 651 tests pass on the rebased branch, ruff lint and format clean (ruff lint and format, ty, deps-check). No new dependency.
• The URL-cache test was mutation-checked. Asserting that two requests return the same URL is not enough, because two signatures taken in the same wall-clock second are byte-identical, so such a test passes even with the cache deleted. The test counts presign calls instead, and bypassing the cache does make it fail.
• The LRU eviction test was mutation-checked the same way (replacing popitem with clear fails it).

Known limits of the URL cache

Both raised in review and tracked separately, neither blocking:

• URL stability is per-worker (#671). The cache is in-process, so with W workers a client sees up to W distinct URLs per frame and fetches it up to W times instead of once. Still much better than re-signing on every poll, just divided by W. An earlier version of this PR claimed the repo runs a single worker; that was inferred from the dev compose files passing no --workers, production runs several, and its deployment config is not in this repo. Corrected in the docstring.
• Stale-window entries stay resident (#672). The window slot is part of the cache key, so prior-window entries are unreachable but occupy space until size-evicted, putting effective capacity at roughly half the nominal 8192. Purging them is safe, unlike the full clear() the code comment argues against.

These matter more once the frontend starts requesting large limit values: a 1000-detection load burns ~2000 entries against a 8192 bound, so a handful of sequences evict each other and the cache stops paying off.

Depends on

#664 for the detections(sequence_id, created_at) index. This PR is functionally independent and its tests pass without it, but the sequence read stays on a sequential scan until it lands.

Out of scope

• GET /detections/ is an unbounded fetch_all with no limit or offset at all. Real problem, separate issue.
• Backfilling Cache-Control onto already-uploaded objects.
• The naming inconsistency between desc here and order_desc on the alerts endpoints.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.83%. Comparing base (5c73e72) to head (7c6034c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   93.73%   93.83%   +0.10%     
==========================================
  Files          59       59              
  Lines        3143     3197      +54     
==========================================
+ Hits         2946     3000      +54     
  Misses        197      197              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Acruve15]

Rebased to drop the detections(sequence_id, created_at) migration: it turned out to be exactly one of the three indexes proposed in #663, so it now lives in #664 with a single owner. This PR depends on #664 for its DB numbers, though it is functionally independent and its tests pass without it.

[fe51]

LGTM on the approach (I need to test it soon)
The interval-based choice over target-count is the right one, and the key design decision, row_number() computed ascending from the start of the sequence regardless of desc (src/app/crud/crud_detection.py:43-52), is what makes the sampled frame set stable as a sequence grows. Combined with the stable presigned URLs, that's what lets a polling player actually hit its cache.

The two commits are more coupled than the split suggests: without sampling, a full 1,000-detection load burns ~2,000 URL cache entries and the cache is effectively useless.

To communicate to the frontend before they build against this

sampling and limit are independent. ?sampling=10 alone, with the desc=true / limit=10 defaults, returns the 10 most recent sampled frames — not a spread across the sequence. Full coverage needs limit >= ceil(detections_count / sampling), and limit caps at 500 (so sampling >= detections_count / 500).

Worked example for a 20/20/20 player load on a 1,000-detection sequence:

?desc=false&limit=20                        # first 20
?desc=false&sampling=48&offset=1&limit=20   # 20 across the middle
?desc=true&limit=20                         # last 20, reverse client-side

General form for M middle frames: sampling = floor((N - 40) / M), offset = ceil(20 / sampling), limit = M. detections_count is already returned on the sequence object, so the frontend has everything it needs — no extra endpoint required.

Non-blocking follow-ups

Neither is a problem today; both are tracked separately so they don't hold this PR.

• #672: purge stale-window cache entries. The window slot is part of the cache key (src/app/services/storage.py:155), so entries from previous windows are permanently unreachable but stay resident until size-evicted. Effective capacity is roughly half the nominal 8192. Purging them is strictly safe — unlike a full clear(), which the existing comment rightly argues against.

• #671: cache sizing under multiple workers. _URL_CACHE_MAXSIZE is per-S3Bucket, and bucket instances are now process-lifetime (src/app/services/storage.py:208), so the bound is n_workers × n_active_orgs × ~8 MB rather than flat. Since we run multiple workers in prod, URL stability is also per-worker: a client sees up to W distinct URLs per frame, so each frame downloads up to W times instead of once. Still a large win over re-signing on every poll, just divided by W.

[Acruve15]

Thanks, both follow-ups are fair and the worker one corrects something I had wrong. Pushed 76ee7fd.

The sampling / limit trap is now documented in both the endpoint description and the client docstring: that they are independent, that spanning the sequence needs limit >= ceil(detections_count / sampling) (hence sampling >= detections_count / 500), and that ?sampling=10 alone returns the most recent sampled frames rather than a spread.

I verified your 20/20/20 recipe against the endpoint rather than just the arithmetic, on a seeded 1,000-detection sequence:

first  : 1..20    n=20
middle : 49..961  n=20  step=48
last   : 981..1000 n=20
overlap first/middle=[] middle/last=[]
total distinct frames=60 of 60

Your formula is exactly right (sampling=48, offset=1), and the warning reproduces: ?sampling=10 alone returns [901, 911, ..., 991], the tail. The coverage rule also checks out, ?sampling=10&limit=100 gives 100 frames spanning 1..991 at step 10.

On workers, you are right and my docstring was wrong. I had asserted "the repo currently runs a single worker (docker-compose.yml passes no --workers)". That was inferred from the dev compose files, and the production deployment is not in this repo, so I should not have made the claim. The docstring now states the per-worker consequence plainly (up to W distinct URLs per frame, so up to W fetches instead of one, still better than re-signing every poll but divided by W) and points at #671. The PR body is updated too.

On #672, agreed, and it is a consequence of my choosing to put the window slot in the cache key instead of clearing on rollover. Purging unreachable prior-window entries is strictly safe as you say, and it roughly doubles effective capacity. Left out of this PR since you have it tracked.

On the coupling between the two commits: agreed, and your framing is sharper than mine. Without sampling, a 1,000-detection load burns ~2,000 entries against a 8,192 bound, so ~4 sequences evict each other and the cache stops paying off. Worth noting that makes #672 and #671 more than cosmetic once the frontend starts requesting large limit values.

🤖 Addressed by Claude Code

[fe51]

Thanks for the follow-ups — the omitted-limit default plus the two headers is a good answer to the truncation trap, and the worker docstring reads accurately now.

One thing I'd like to discuss before this merges, on offset. Not a bug report — the current behaviour is self-consistent and the tests are right. It's a question about which semantics we want to commit to, and I'd rather raise it now than after the frontend builds on it.

What I noticed

offset used to mean one thing: how many detections to skip in the sequence. With sampling, it now counts in sampled frames, so its meaning depends on the value of another parameter. The description string had to gain a qualifier to say so — "Number of detections to skip, within the sampled set".

Concretely, on the 1,000-detection sequence from your verification:

?sampling=48&offset=20   →  starts at detection 961

Twenty is read as twenty grid points, so it skips 20 × 48 = 960 detections and lands near the end. The flip side is that skipping the first 20 detections requires offset=1, which is the conversion in the recipe we landed on: offset = ceil(20 / sampling).

What I'd expect instead

offset keeps meaning "how many detections I skip", and sampling then applies from that point:

?sampling=48&offset=20   →  21, 69, 117, … 933

Two reasons I lean this way:

1. It doesn't redefine an existing parameter. Today the same offset means different things depending on whether sampling is set, which is the kind of thing that reads fine in the docstring and bites at 3am. Under the proposal offset has one meaning, and for sampling=1 the two readings are identical anyway.
2. It removes the conversion from every caller. The frontend recipe becomes sampling = floor((N - 40) / M), offset = 20, limit = M — no ceil(), nothing to get subtly wrong at small sampling values.

What it seems to cost

From reading fetch_by_sequence, the predicate would become rn - 1 >= offset AND (rn - 1 - offset) % sampling == 0, with offset moving out of the SQL OFFSET and into the WHERE. sampled_total becomes ceil((count - offset) / sampling), which arguably makes the headers more useful since they'd then describe the span from where the caller actually starts.

As far as I can tell the two properties that matter both survive:

• Stability — rn is still numbered ascending from the start of the sequence, so a new detection renumbers nothing, and the same offset + sampling returns the same frames poll after poll.
• Pagination — still works, you advance offset += limit × sampling instead of offset += limit.

The question

Is there something that makes this more expensive than it looks, or a case I'm not seeing where counting in sampled units is the more useful behaviour? You've been closer to this query than I have.

If it's not technically limiting, this feels like the more natural shape, and now is the cheapest possible moment to pick it — sampling hasn't shipped, so no caller depends on the current reading, and sampling=1 behaves identically either way.

(There's a middle option, keeping a sequence-wide grid and treating offset as a raw floor onto it, so offset=20&sampling=48 would start at 49 rather than 21. It's a smaller change — the conversion is just ceil(offset / sampling) server-side — but it keeps a rounding step that callers have to know about, so I'd rather have the clean version if the cost is comparable.)

@Acruve15 @MateoLostanlen what do you think ?