Allow users to customize the time gap between sessions#1292
Open
Allow users to customize the time gap between sessions#1292
Conversation
The session_time_gap_seconds project setting was added in #918 but never used. group_images_into_events() still hardcoded a 120-minute gap. This wires the field through and exposes it in the Processing settings UI so each project can override the session boundary. Closes #906 Co-Authored-By: Claude <noreply@anthropic.com>
POST /api/v2/deployments/<pk>/regroup-sessions/ queues the existing ami.tasks.regroup_events Celery task for the deployment. The task calls group_images_into_events(), which now reads the project's session_time_gap_seconds setting. User-facing terminology is "sessions"; "Event" is retained as the internal model name for backwards compatibility. Returns 202 with task_id, deployment_id, project_id. Refs #906 Co-Authored-By: Claude <noreply@anthropic.com>
Three new TestImageGrouping cases that exercise the new session_time_gap_seconds wiring: - test_cross_midnight_bursts_split_by_short_gap: documents the user- reported pain (2 bursts with off-window > default gap, crossing midnight, split into 2 Events on different dates) and proves that bumping max_time_gap past the off-window collapses them to 1 Event. - test_same_date_bursts_merge_regardless_of_gap: documents the inverse behavior — bursts on the same calendar date collide on group_by regardless of gap setting. Acts as a regression target for any future rework of the date-keyed Event reuse path (per the #904 caveat at models.py:1516). - test_session_time_gap_seconds_is_used_when_no_explicit_gap: verifies group_images_into_events falls back to the project setting when no max_time_gap is passed, including refreshing the cached relation after the setting changes. Refs #906 Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
Contributor
📝 WalkthroughWalkthroughThis PR makes the session time gap threshold configurable per project, adds a regroup-sessions API endpoint to enqueue regrouping, updates grouping logic to use project settings with sensible fallbacks, adds a UI form field, creates a migration and permission, and adds tests for grouping and idempotency. ChangesSession Grouping Configuration
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Celery as Worker
participant Cache
participant DB
Client->>API: POST /deployments/{id}/regroup-sessions
API->>Worker: enqueue regroup_events(deployment_id)
API-->>Client: 202 Accepted + task_id
Worker->>Cache: cache.add(regroup_lock_deployment_{id}, ttl)
alt lock acquired
Worker->>DB: load Deployment, call group_images_into_events(...)
DB-->>Worker: grouped events result
Worker->>Cache: cache.delete(regroup_lock_deployment_{id})
else lock exists
Worker-->>Worker: log and return early
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mihow
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/api/views.py`:
- Around line 320-323: The view currently enqueues regroup_events_task
unconditionally allowing overlapping runs; modify the handler that calls
self.get_object() and regroup_events_task.delay(deployment.pk) to acquire a
per-deployment in-flight lock (e.g., a Redis/cache-based lock keyed by
f"regroup:{deployment.pk}") before queuing; if the lock cannot be obtained
return an early HTTP 409 (or similar) Response and log that a regroup is already
running, otherwise enqueue the task, keep the lock for the duration of the task
(or set a TTL and have the task release the lock) and ensure regroup_events_task
(or its wrapper) releases the lock on completion/failure so duplicate triggers
are prevented.
In `@ui/src/pages/project-details/processing-form.tsx`:
- Around line 30-35: The sessionTimeGapSeconds field currently validates but
will submit a string because React Hook Form doesn't coerce number inputs;
update the field's rules for sessionTimeGapSeconds to include a setValueAs that
converts the incoming value to a number (e.g., Number or parseInt) so the form
value is stored as a numeric type; modify the rules object for
sessionTimeGapSeconds to add setValueAs and keep required/min validations
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80284157-fc1e-41e0-8398-96a13aa37201
📒 Files selected for processing (4)
ami/main/api/views.pyami/main/models.pyami/main/tests.pyui/src/pages/project-details/processing-form.tsx
Contributor
There was a problem hiding this comment.
Pull request overview
This PR wires the existing per-project session_time_gap_seconds setting into backend session (Event) grouping, exposes the setting in the UI, and adds an API action to trigger regrouping asynchronously so admins can re-cluster historical captures after changing the threshold.
Changes:
- Backend:
group_images_into_events()now derives its defaultmax_time_gapfromdeployment.project.session_time_gap_secondswhen not explicitly provided. - API: adds
POST /api/v2/deployments/<pk>/regroup-sessions/to enqueue the existingami.tasks.regroup_eventsCelery task. - UI + tests: exposes the setting as a numeric input on the project Processing settings form and adds unit tests covering cross-midnight behavior and the project-setting fallback.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
ami/main/models.py |
Uses the project’s session_time_gap_seconds as the default grouping gap instead of a hardcoded 120 minutes. |
ami/main/api/views.py |
Adds a deployment-level action endpoint to queue regrouping in Celery. |
ui/src/pages/project-details/processing-form.tsx |
Adds a numeric form field to edit sessionTimeGapSeconds in the Processing settings UI. |
ami/main/tests.py |
Adds tests documenting cross-midnight split/merge behavior and verifying the project setting is used when no explicit gap is passed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…oup task Addresses takeaway-review feedback on PR #1292: - Add `regroup_sessions_deployment` custom Project permission so non-superusers with the ProjectManager role can hit `POST /deployments/<pk>/regroup-sessions/` (mirrors `sync_deployment`). Without this, `BaseModel.check_custom_permission` computes the codename `regroup_sessions_deployment` and the perm doesn't exist, so the action 403s for everyone except superusers. - Wrap `ami.tasks.regroup_events` in a per-deployment cache lock. Concurrent enqueues (double-clicks, sync_captures auto-regroup racing with manual trigger) collapse to a single run instead of overlapping on the same rows. Lower-level than the view so all callers (Deployment.save, admin bulk action, new API action) are protected. - Defensive clamp on `session_time_gap_seconds` — fall back to the historical 120-minute default if the value is 0 / negative, with a warning log. Stops pathological "every gap = new event" behavior when admins enter bad values. - Migration 0085 adds the new perm to `Project.Meta.permissions` and grants it to existing `ProjectManager` role groups via a data migration (mirrors how 0084 revoked `delete_job`). - Tests: - test_invalid_session_time_gap_falls_back_to_default exercises 0 / -1 / -7200 - test_regroup_events_task_is_idempotent_under_concurrent_calls pre-takes the cache lock, asserts a second call skips group_images_into_events Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/tasks.py`:
- Around line 106-119: The current lock uses cache.add(lock_key, 1, ...) and
unconditionally cache.delete(lock_key) which can remove a newer owner's lock;
change to an ownership token pattern: when acquiring the lock (where cache.add
is called) generate a unique token (e.g. UUID) and store that token as the cache
value instead of 1 (use REGROUP_EVENTS_LOCK_TTL as before), keep the token in a
local variable, and on release only remove the key if the stored cache value
equals your token (fetch with cache.get(lock_key) and only call
cache.delete(lock_key) when it matches) or use the cache backend's atomic
compare-and-delete if available; update the lock handling around lock_key,
cache.add, cache.get and cache.delete to use this token ownership check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 251316be-c276-4473-a3fc-36a150e056ed
📒 Files selected for processing (5)
ami/main/migrations/0085_add_regroup_sessions_deployment_permission.pyami/main/models.pyami/main/tests.pyami/tasks.pyami/users/roles.py
✅ Files skipped from review due to trivial changes (1)
- ami/users/roles.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ami/main/models.py
Comment on lines
+106
to
+119
| if not cache.add(lock_key, 1, timeout=REGROUP_EVENTS_LOCK_TTL): | ||
| logger.info(f"regroup_events skipped for deployment {deployment_id}: another run is in progress.") | ||
| return | ||
| logger.info(f"Grouping captures for {deployment}") | ||
| events = group_images_into_events(deployment) | ||
| logger.info(f"{deployment} now has {len(events)} events") | ||
| try: | ||
| try: | ||
| deployment = Deployment.objects.get(id=deployment_id) | ||
| except Deployment.DoesNotExist: | ||
| logger.error(f"Deployment with id {deployment_id} not found") | ||
| return | ||
| logger.info(f"Grouping captures for {deployment}") | ||
| events = group_images_into_events(deployment) | ||
| logger.info(f"{deployment} now has {len(events)} events") | ||
| finally: | ||
| cache.delete(lock_key) |
Contributor
There was a problem hiding this comment.
Lock release can delete a newer lock owner
The finally: cache.delete(lock_key) path can clear a lock acquired by a later task when the first lock expires mid-run. That re-opens concurrent regroup execution and breaks the idempotency guarantee.
Suggested hardening
+import uuid
...
- lock_key = f"regroup_events:lock:deployment:{deployment_id}"
- if not cache.add(lock_key, 1, timeout=REGROUP_EVENTS_LOCK_TTL):
+ lock_key = f"regroup_events:lock:deployment:{deployment_id}"
+ lock_token = uuid.uuid4().hex
+ if not cache.add(lock_key, lock_token, timeout=REGROUP_EVENTS_LOCK_TTL):
logger.info(f"regroup_events skipped for deployment {deployment_id}: another run is in progress.")
return
try:
...
finally:
- cache.delete(lock_key)
+ if cache.get(lock_key) == lock_token:
+ cache.delete(lock_key)For full safety, use an ownership-checked atomic unlock primitive in the cache backend (token compare-and-delete).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ami/tasks.py` around lines 106 - 119, The current lock uses
cache.add(lock_key, 1, ...) and unconditionally cache.delete(lock_key) which can
remove a newer owner's lock; change to an ownership token pattern: when
acquiring the lock (where cache.add is called) generate a unique token (e.g.
UUID) and store that token as the cache value instead of 1 (use
REGROUP_EVENTS_LOCK_TTL as before), keep the token in a local variable, and on
release only remove the key if the stored cache value equals your token (fetch
with cache.get(lock_key) and only call cache.delete(lock_key) when it matches)
or use the cache backend's atomic compare-and-delete if available; update the
lock handling around lock_key, cache.add, cache.get and cache.delete to use this
token ownership check.
Collaborator
Author
|
Claude says: Push 50d618e addresses the takeaway-review feedback from CodeRabbit and Copilot:
New tests covering the lock and the clamp:
11/11 |
annavik
reviewed
May 7, 2026
Member
There was a problem hiding this comment.
Nice!!
I still think this setting would make more sense in a new section, instead of in Project->Settings->Processing. I know you said this can affect the logic for occurrences once tracking is enabled, but I think this is a bit far-fetched. I'm thinking, if I were about to look for this setting as a user, where would I first go? I'm not sure the processing section would be the place. Maybe this belongs more with upcoming sampling schedule configuration we have discussed?
I don't have very strong opinions here though, so if you feel this is where it belongs, go for it! :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #906.
Summary
The
session_time_gap_secondsproject setting was added in #918 but was never read by anything —group_images_into_events()still hardcoded a 120-minute gap, so changing the field had no effect.This PR:
deployment.project.session_time_gap_secondsinsidegroup_images_into_events()when no explicitmax_time_gapis passed. Falls back to 120 minutes if the deployment has no project, or if the configured value isNone/0/ negative (with a warning log). Seeami/main/models.py:1395-1414.ui/src/pages/project-details/processing-form.tsx) so admins can edit it without touching Django admin.POST /api/v2/deployments/<pk>/regroup-sessions/action that queues the existingami.tasks.regroup_eventsCelery task. Lets admins re-trigger session grouping after changing the project setting (the existing auto-regroup only fires when new/ungrouped images are detected, so a setting change alone won't trigger it).regroup_sessions_deploymentcustom Project permission, granted to theProjectManagerrole, so non-superusers can hit the new endpoint. Mirrors howsync_deploymentis wired.ami.tasks.regroup_eventsin a per-deployment cache lock (cache.add(), 1 h TTL). Concurrent enqueues — double-clicks,Deployment.save()autoregroup racing with a manual admin trigger, etc. — collapse to a single run instead of overlapping on the same rows. Lower-level than the view, so all callers are protected (new API action, autoregroup, admin bulk action).TestImageGroupingtests covering cross-midnight burst patterns, same-date merging behavior, the project-setting fallback, the non-positive clamp, and the lock idempotency.One DB migration (
0085_add_regroup_sessions_deployment_permission) registers the new custom permission and grants it to existingProjectManagerrole groups via a data migration. Thesession_time_gap_secondsfield itself already existed with default7200seconds (= 2 hours = 120 minutes), matching the previous hardcoded behavior.Use case this fixes
A user reported that monitoring schedules with multi-burst nights (e.g. 2 hours on, 2 hours off, 2 hours on) sometimes group into one Event and sometimes into two, depending on whether the off-window crosses midnight. With this PR, raising the gap setting past the longest off-window collapses cross-midnight bursts into a single Event consistently.
The new test
test_cross_midnight_bursts_split_by_short_gapdocuments this: at the default 2 h gap, two bursts spanning midnight produce 2 Events; at a 4 h gap the same captures produce 1 Event.API usage
Request
No request body required.
Response
202 Accepted:{ "task_id": "f3a1c2b8-9d4e-4f7a-bc01-2e3a4b5c6d7e", "deployment_id": 74, "project_id": 24 }The response returns immediately; regrouping runs as a background Celery task. On the test deployment used here (~1.2 k captures), each task completed in 14–22 seconds. Track progress via Flower (
http://<host>:5555) or the worker logs.If a regroup is already in progress for the same deployment, subsequent calls still return
202with a freshtask_id, but the task short-circuits at the worker (logged:regroup_events skipped for deployment <id>: another run is in progress.) without touching the DB.Error responses
404 Not Found— deployment pk does not exist or is not visible to the requesting user.401 Unauthorized/403 Forbidden— token missing or user lacks theregroup_sessions_deploymentpermission on the deployment's project (granted to theProjectManagerrole and superusers).Caveats — what this PR does and doesn't change
The setting is now wired through, but Event keying still collapses some timestamp groups. Below are the scenarios actually tested and what they show.
Production data — 5-setting cycle on a real deployment
Cycled
session_time_gap_secondsthrough five values via the new endpoint, on a deployment with ~1.2 k captures currently grouped into 69 Events at the default 2 h gap. Each task completed in 14–22 s.Reading: tightening below ~24 h produced no observable Event-count change on this deployment. Loosening to 24 h merged 9 cross-midnight nights (−13 %).
Why tightening doesn't always change Event count
group_images_into_events()keys Events bygroup_by = start_date.date()and usesEvent.objects.get_or_create(deployment, group_by)(ami/main/models.py:1455-1460). Two timestamp groups produced by a tighter gap that start on the same calendar date collide back into a single Event. So the gap setting only changes Event count when:The existing comment at
models.py:1516already flags this: "#904 is expected to rework this reuse path more thoroughly."Synthetic unit tests added in this PR
ami/main/tests.py::TestImageGrouping:test_cross_midnight_bursts_split_by_short_gaptest_same_date_bursts_merge_regardless_of_gaptest_session_time_gap_seconds_is_used_when_no_explicit_gaptest_invalid_session_time_gap_falls_back_to_defaulttest_regroup_events_task_is_idempotent_under_concurrent_callsgroup_images_into_eventsnot invokedWhat this PR proves on its own
group_images_into_eventsreads it).group_byfield and replaces date-keyed reuse with time-overlap matching. This PR is the prerequisite that lets Fix incorrect session grouping #904's reuse-path rework actually act on a configurable threshold.Related PRs
group_byfield, addsuse_existingflag, replaces unique constraint. Complementary to this PR: Fix incorrect session grouping #904 makes the gap setting visibly meaningful below the day boundary.docs/claude/planning/session-grouping-strategies.mdfor the broader inventory.Test plan
yarn builddocker compose build django uimanage.py test ami.main.tests.TestImageGrouping— 11/11 pass (5 new + 6 existing)manage.py test ami.main.tests.TestProjectPermissions ami.main.tests.TestRolePermissions— 13/13 pass (no regression from the new permission)POST /regroup-sessions/. Endpoint returns 202, tasks complete in 14–22 s, 24 hr setting confirmed to merge cross-midnight nights (see table above).Notes / follow-ups
"7200") because<input type="number">returns DOM strings. DRFIntegerFieldparses it transparently. Same pattern is used in 3 other forms in this codebase (capture-set, deployment-location, job-details). Standardizing the form helper to coerce numbers is a separate cleanup, not addressed here.regroup-sessionsaction could be surfaced as a row-action on the Stations list and as a button on the project settings screen — separate UI follow-up.deployment_events_need_update()does not detect a setting change as a reason to regroup, so admins must hit the new API action (or the Django admin bulk action) after editing the gap. Auto-detection on setting change is a possible follow-up.regroup_eventsin the Job framework so admins see progress/logs in the Jobs UI — exploratory plan parked atdocs/claude/planning/regroup-events-job-wrapper.md.Co-author
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
New Features
Improvements
Permissions
Tests