refactor(store): wave F — reactive triggers → Go listeners (#301)#302
Merged
Conversation
Drops the four PL/pgSQL reactive triggers and their helper functions
that fired dynamic-group re-evaluation on side-table changes. The
equivalent enqueueing moves into the Go projector listeners that
already process the underlying events.
Migration 049 drops:
- device_labels_change_trigger (on device_labels)
- device_deleted_trigger (on devices_projection)
- device_inventory_changed (on device_inventory)
- user_attribute_change_trigger (on users_projection)
Plus trigger_device_labels_child_change, trigger_device_label_change
(legacy from JSONB days), trigger_device_deleted, trigger_inventory_change,
queue_dynamic_groups_for_device, queue_dynamic_user_groups_on_user_change,
queue_all_dynamic_groups.
Go wiring:
- New sqlc queries: EnqueueAllDynamicDeviceGroups(reason),
EnqueueAllDynamicUserGroups(reason), ListDeviceGroupMembershipsByDevice,
DeleteDeviceGroupMembershipsForDevice. The reason text moves from
SQL-side string concat into the Go caller.
- enqueueDynamicDeviceGroupsForDevice / enqueueDynamicUserGroupsForUser
helpers in the projector package. Reason format preserved
('device_<id>_changed' / 'user_<id>_changed').
- Device projector wires the helper into applyDeviceLabelSet /
applyDeviceLabelRemoved / applyDeviceLabelsUpdated / applyDeviceRegistered.
- Device-deleted listener cascades: list affected group_ids, delete
device_group_members rows, recount only the affected groups
(scope-tightening vs the dropped trigger's full-table sweep).
- Inbox-worker's handleInventoryUpdate explicitly enqueues device
groups after the inventory upsert chain (replaces device_inventory_changed).
- User projector wires the helper into applyUserCreatedWithRoles /
applyUserProfileUpdated / applyUserEmailChanged /
applyUserPasswordChanged / applyUserDisabled / applyUserEnabled /
applyUserDeleted. TOTP listener fires it on TOTPVerified / TOTPDisabled.
- cmd/control/periodic.go's QueueAllDynamicGroups call replaced with
the two new typed enqueue queries (one device-side, one user-side)
so the safety-net sweep still primes both queues every 24h.
- QueueAllDynamicGroups sqlc shim deleted (callerless after the
periodic rewire).
Refs #242.
📝 WalkthroughWalkthroughReactive PostgreSQL triggers that re-evaluated dynamic groups on device/user mutations are removed and migrated to explicit Go enqueue calls in the event projectors. New sqlc queries bulk-enqueue device and user groups with caller-supplied reasons; device deletion cascade is expanded; periodic and inventory-worker re-eval paths are updated to use the split typed enqueue queries. ChangesWave F: Reactive trigger removal and Go listener enqueue migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@cmd/control/periodic.go`:
- Around line 117-124: The success log "queued full dynamic group re-evaluation"
is emitted even if EnqueueAllDynamicDeviceGroups fails; change the logic in the
block that calls st.Queries().EnqueueAllDynamicDeviceGroups(ctx, reason) and
st.Queries().EnqueueAllDynamicUserGroups(ctx, reason) so the logger.Info is only
called when both calls return nil: capture each error (from
EnqueueAllDynamicDeviceGroups and EnqueueAllDynamicUserGroups), log individual
failures with logger.Error as currently done, and only call logger.Info("queued
full dynamic group re-evaluation") if both errors are nil (i.e., both enqueues
succeeded).
In `@internal/control/inbox_worker.go`:
- Around line 492-497: The call to
w.store.Queries().EnqueueAllDynamicDeviceGroups currently only logs a warning
and swallows the error, preventing the task from being retried; change the
handler so that when EnqueueAllDynamicDeviceGroups (the call with key
"device_"+payload.DeviceID+"_changed") returns a non-nil error you log it
(optional) and then return that error (or wrap it) from the enclosing handler
function so Asynq will retry the task instead of treating it as success.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c39ae573-78c1-4f91-8967-ff3fb907dda4
⛔ Files ignored due to path filters (2)
internal/store/generated/device_groups.sql.gois excluded by!**/generated/**internal/store/generated/user_groups.sql.gois excluded by!**/generated/**
📒 Files selected for processing (8)
cmd/control/periodic.gointernal/control/inbox_worker.gointernal/projectors/device_listener.gointernal/projectors/totp_listener.gointernal/projectors/user_listener.gointernal/store/migrations/049_drop_reactive_triggers.sqlinternal/store/queries/device_groups.sqlinternal/store/queries/user_groups.sql
Comment on lines
+117
to
124
| if err := st.Queries().EnqueueAllDynamicDeviceGroups(ctx, reason); err != nil { | ||
| logger.Error("failed to queue full dynamic device-group re-evaluation", "error", err) | ||
| } | ||
| if err := st.Queries().EnqueueAllDynamicUserGroups(ctx, reason); err != nil { | ||
| logger.Error("failed to queue full dynamic user-group re-evaluation", "error", err) | ||
| } else { | ||
| logger.Info("queued full dynamic group re-evaluation") | ||
| } |
There was a problem hiding this comment.
Success log can be false-positive when device enqueue fails.
On Line 123, the success message is emitted when only the user enqueue succeeds; a failure on Line 117 still results in an overall “queued full dynamic group re-evaluation” log. Gate the success log on both calls succeeding.
Suggested fix
const reason = "periodic_full_evaluation"
- if err := st.Queries().EnqueueAllDynamicDeviceGroups(ctx, reason); err != nil {
+ deviceQueued := true
+ userQueued := true
+ if err := st.Queries().EnqueueAllDynamicDeviceGroups(ctx, reason); err != nil {
+ deviceQueued = false
logger.Error("failed to queue full dynamic device-group re-evaluation", "error", err)
}
if err := st.Queries().EnqueueAllDynamicUserGroups(ctx, reason); err != nil {
+ userQueued = false
logger.Error("failed to queue full dynamic user-group re-evaluation", "error", err)
- } else {
+ }
+ if deviceQueued && userQueued {
logger.Info("queued full dynamic group re-evaluation")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := st.Queries().EnqueueAllDynamicDeviceGroups(ctx, reason); err != nil { | |
| logger.Error("failed to queue full dynamic device-group re-evaluation", "error", err) | |
| } | |
| if err := st.Queries().EnqueueAllDynamicUserGroups(ctx, reason); err != nil { | |
| logger.Error("failed to queue full dynamic user-group re-evaluation", "error", err) | |
| } else { | |
| logger.Info("queued full dynamic group re-evaluation") | |
| } | |
| deviceQueued := true | |
| userQueued := true | |
| if err := st.Queries().EnqueueAllDynamicDeviceGroups(ctx, reason); err != nil { | |
| deviceQueued = false | |
| logger.Error("failed to queue full dynamic device-group re-evaluation", "error", err) | |
| } | |
| if err := st.Queries().EnqueueAllDynamicUserGroups(ctx, reason); err != nil { | |
| userQueued = false | |
| logger.Error("failed to queue full dynamic user-group re-evaluation", "error", err) | |
| } | |
| if deviceQueued && userQueued { | |
| logger.Info("queued full dynamic group re-evaluation") | |
| } |
🤖 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 `@cmd/control/periodic.go` around lines 117 - 124, The success log "queued full
dynamic group re-evaluation" is emitted even if EnqueueAllDynamicDeviceGroups
fails; change the logic in the block that calls
st.Queries().EnqueueAllDynamicDeviceGroups(ctx, reason) and
st.Queries().EnqueueAllDynamicUserGroups(ctx, reason) so the logger.Info is only
called when both calls return nil: capture each error (from
EnqueueAllDynamicDeviceGroups and EnqueueAllDynamicUserGroups), log individual
failures with logger.Error as currently done, and only call logger.Info("queued
full dynamic group re-evaluation") if both errors are nil (i.e., both enqueues
succeeded).
Comment on lines
+492
to
+497
| if err := w.store.Queries().EnqueueAllDynamicDeviceGroups(ctx, "device_"+payload.DeviceID+"_changed"); err != nil { | ||
| w.logger.Warn("failed to enqueue dynamic device groups after inventory update", | ||
| "device_id", payload.DeviceID, | ||
| "error", err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Do not swallow enqueue failures; return an error to retry the task.
If Line 492 fails, the handler only warns and still returns success, so this inventory change may never enqueue dynamic-group re-evaluation promptly. Return an error here so Asynq retries and preserves expected behavior.
Suggested fix
if err := w.store.Queries().EnqueueAllDynamicDeviceGroups(ctx, "device_"+payload.DeviceID+"_changed"); err != nil {
- w.logger.Warn("failed to enqueue dynamic device groups after inventory update",
- "device_id", payload.DeviceID,
- "error", err,
- )
+ return fmt.Errorf("enqueue dynamic device groups after inventory update for device %s: %w", payload.DeviceID, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := w.store.Queries().EnqueueAllDynamicDeviceGroups(ctx, "device_"+payload.DeviceID+"_changed"); err != nil { | |
| w.logger.Warn("failed to enqueue dynamic device groups after inventory update", | |
| "device_id", payload.DeviceID, | |
| "error", err, | |
| ) | |
| } | |
| if err := w.store.Queries().EnqueueAllDynamicDeviceGroups(ctx, "device_"+payload.DeviceID+"_changed"); err != nil { | |
| return fmt.Errorf("enqueue dynamic device groups after inventory update for device %s: %w", payload.DeviceID, err) | |
| } |
🤖 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 `@internal/control/inbox_worker.go` around lines 492 - 497, The call to
w.store.Queries().EnqueueAllDynamicDeviceGroups currently only logs a warning
and swallows the error, preventing the task from being retried; change the
handler so that when EnqueueAllDynamicDeviceGroups (the call with key
"device_"+payload.DeviceID+"_changed") returns a non-nil error you log it
(optional) and then return that error (or wrap it) from the enclosing handler
function so Asynq will retry the task instead of treating it as success.
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
1 participant
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.
Part of the storage-abstraction tracker (#242), Wave F. Drops the four PL/pgSQL reactive triggers that fired dynamic-group re-evaluation on side-table changes; moves the enqueueing into the Go projector listeners that already process the underlying events.
Closes #301.
Summary
Migration 049 drops:
Plus 7 supporting PL/pgSQL functions (trigger handlers + queue helpers).
Go wiring
DoD checks
After Wave F lands, the only remaining roadmap item before validation-backend / NoSQL work is Wave G (EventStore interface).
Summary by CodeRabbit
Release Notes