Skip to content

fix(sequences): start sequences at the first detection's recorded_at - #687

Open
MateoLostanlen wants to merge 5 commits into
mainfrom
feat/sequence-recorded-at
Open

fix(sequences): start sequences at the first detection's recorded_at#687
MateoLostanlen wants to merge 5 commits into
mainfrom
feat/sequence-recorded-at

Conversation

@MateoLostanlen

@MateoLostanlen MateoLostanlen commented Sep 3, 2026

Copy link
Copy Markdown
Member
  • Sequences started at the server insertion time of their first detection while detections display recorded_at, so the platform showed two different times for the same event (Fix sequence creation timestamps to rely on recorded_at #675).
  • started_at now takes the first detection's recorded_at; the rest of the linking logic and last_seen_at are unchanged.
  • A data-only migration realigns existing sequences on their earliest detection's recorded_at (no-op where both already match).

Closes #675

Sequences expose the capture time of the detection that started them, so
the platform can show one consistent time alongside detections. The
migration backfills existing rows from their earliest detection and falls
back to started_at.
…d_at

Drop the separate recorded_at column: started_at itself now carries the
capture time of the first detection. The migration becomes data-only and
realigns existing sequences on their earliest detection.
from alembic import op

# revision identifiers, used by Alembic.
revision: str = "d6f3a8b2c4e1"

# revision identifiers, used by Alembic.
revision: str = "d6f3a8b2c4e1"
down_revision: Union[str, None] = "c4e9f1a2b3d5"
# revision identifiers, used by Alembic.
revision: str = "d6f3a8b2c4e1"
down_revision: Union[str, None] = "c4e9f1a2b3d5"
branch_labels: Union[str, Sequence[str], None] = None
revision: str = "d6f3a8b2c4e1"
down_revision: Union[str, None] = "c4e9f1a2b3d5"
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None
@MateoLostanlen MateoLostanlen changed the title feat(sequences): expose recorded_at for consistent display fix(sequences): start sequences at the first detection's recorded_at Sep 3, 2026
@MateoLostanlen
MateoLostanlen requested a review from fe51 September 3, 2026 12:29

@fe51 fe51 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MateoLostanlen ,

Thanks fort the PR. I have noticed some stuff to challenge introducing this small updates, and uses Claude to detailed it. happy to discuss it
The core change is right and minimally scoped: started_at = first_det.recorded_at makes the sequence agree with what the UI already shows for detections, the revision chain stays linear and single-headed, and the new test genuinely fails on the old code.

Four inline comments cover the code changes. The rest below touches files this PR doesn't modify, so it lands here.


Freshness windows use the server clock (last_seen_at); event time and date bucketing use started_at.

That is what makes the asymmetry in this PR deliberate rather than accidental — and it decides every item below.


A. The 24h feed window now sits on a camera clock

fetch_latest_unlabeled_sequences (src/app/api/api_v1/endpoints/sequences.py:179) gates the unlabeled feed with started_at > utcnow() - 24h. After this PR that compares a camera clock against the server clock: a camera with a lagging RTC (booted without NTP) emits live sequences that silently never enter the feed, and nobody thinks to check created_at to find out why.

    stmt: Any = (
        select(Sequence)
        # Freshness window on last_seen_at, not started_at: started_at is now the camera's
        # capture clock, so a camera with a lagging RTC (booted without NTP) would emit live
        # sequences that silently never enter this feed. last_seen_at is always written from
        # the server clock, which keeps the window honest. Same rule as the alert feed.
        .where(Sequence.last_seen_at > utcnow() - timedelta(hours=24))
        .where(Sequence.is_wildfire.is_(None))  # type: ignore[union-attr]
    )

The semantics shift slightly, and in the feed's favour: it becomes "sequences seen in the last 24h" rather than "sequences that started in the last 24h", so a sequence that began 30h ago and is still active shows up. That matches alerts.py:97, which already windows on last_seen_at.

Optional companion, and a visible UX change so it's your call: sequences.py:194 orders by started_at.desc() and limits to 15. A camera clock running ahead pins a bogus row to the top; one running behind buries a real one. order_by(Sequence.last_seen_at.desc()) — "most recently active first" — is the consistent partner to the filter above.


B. Deliberately not changing

  • last_seen_at stays det.created_at (detections.py:646, :690). It is a liveness gate, not a display field: _get_continuity_sequences compares it against utcnow() - 120s (SEQUENCE_CONTINUITY_SECONDS), and sequence matching against utcnow() - 120min. On a camera clock, a routine two-minute upload lag would drop the sequence out of the continuity window and put holes in the temporal model's frame timeline. It is also written unconditionally, with no max() — and recorded_at is not monotonic, so an out-of-order upload would shrink the window. Not a one-word swap.
  • sequences.py:216 (func.date(started_at) == from_date) keeps started_at. That query means "what happened on this day", so event time is the correct basis.
  • overlap.py:348-349 stays as is. Mixed clocks stretch each sequence's interval by the camera lag, making the temporal gate marginally more permissive, but TRIANGULATION_RELAXATION_SECONDS defaults to 30 minutes, which swallows any realistic capture-to-insert lag.

Follow-up, as its own issue: last_seen_at is doing two jobs — server-side liveness gate and displayed end-of-event — which is why the window can't be single-clock today. Splitting them fixes it: keep last_seen_at on the server clock for the three gates, add last_recorded_at (event time) for display, the CSV duration and overlap.py. Schema change + migration + three call sites, so not this PR.


C. On the migration

The revision chain is clean — linear, single head (c4e9f1a2b3d5 → d6f3a8b2c4e1), no branch. The s.started_at <> d.recorded_at guard keeping the write set to genuinely-changed rows is the right instinct, and the single DISTINCT ON pass is the correct shape given detections.sequence_id is unindexed: a correlated subquery per sequence would be far worse.

Two things to know before running it in prod. It is a full scan and sort of detections (the largest table), and Alembic wraps it in one transaction, so every sequences row it updates stays locked until commit. Only recently-active sequences contend with ingestion, but if the migration runs 20 minutes, an unlucky detection POST blocks for 20 minutes. The sort may also spill to disk.

So: run SELECT count(*) FROM detections; first. Under ~10M rows this is likely under a minute and none of the above matters; above that, schedule it off-peak. An index on detections(sequence_id, recorded_at) would turn the scan into an index scan and speed up the ingest path too — but that belongs in the PR already introducing indexes, not this one. Rebasing on it afterwards makes the problem disappear.

downgrade as a no-op is defensible for a data-only migration, but it makes this a one-way door: the previous started_at values are gone. Worth a line in the release notes. They stay reconstructible from detections.created_at if anyone ever needs them.

overlapping_dets.append(cand)

if len(overlapping_dets) >= settings.SEQUENCE_MIN_INTERVAL_DETS:
first_det = min(overlapping_dets, key=lambda item: item.created_at)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first_det must be first captured, not first inserted so must rely on recorded_at

Suggested change
first_det = min(overlapping_dets, key=lambda item: (item.recorded_at, item.created_at, item.id))

SELECT DISTINCT ON (sequence_id) sequence_id, recorded_at
FROM detections
WHERE sequence_id IS NOT NULL
ORDER BY sequence_id, created_at, id

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ORDER BY sequence_id, created_at, id
ORDER BY sequence_id, recorded_at, created_at, id

Same ordering key as first_det in create_detection, so backfilled rows match what new code writes.
(so same ordering as suggested in detections.py679)

Comment on lines 573 to 576
# The engine may report when the image was actually captured; fall back to now when it doesn't.
# Stored for display and as a camera-lag signal (created_at - recorded_at); sequence linking still
# keys on created_at (the monotonic server clock). Aware timestamps are normalized to UTC, naive
# ones are assumed UTC, and all rows from a single upload share the same capture time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment is depreciated, recorded_at is not only for display

The engine may report when the image was actually captured; fall back to now when it doesn't. This is the event time the platform displays, and it now sets the sequence's started_at.

assert seq is not None
assert seq.started_at == first_capture


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more test ?

The new test covers capture-vs-insertion time but not the new ordering key. Add a case where the two disagree: post two detections whose recorded_at is descending (second upload carries the earlier capture time, as in a backlog flush) and assert started_at equals the earlier recorded_at

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix sequence creation timestamps to rely on recorded_at

2 participants