Fix 5XXs on software install endpoints from software row locking - #50226
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMaintained app name reconciliation is removed from catalog upserts and added to a separate premium macOS cron schedule. Successful catalog refreshes still trigger best-effort reconciliation. The datastore implementation uses primary reads and bounded, retryable rename batches. Tests cover batching, discovery windows, idempotency, orphaned installers, precedence, multi-team installers, and mobile sibling handling. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50226 +/- ##
==========================================
+ Coverage 62.10% 68.58% +6.48%
==========================================
Files 3986 3986
Lines 256775 256788 +13
Branches 13684 13684
==========================================
+ Hits 159463 176121 +16658
+ Misses 82758 65012 -17746
- Partials 14554 15655 +1101
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
80bc814 to
55518a0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@server/datastore/mysql/maintained_apps.go`:
- Around line 237-243: Update the mismatchedTitlesByIdentifier query to restrict
target software_titles rows to Darwin/macOS sources, excluding ios_apps and
ipados_apps before reconciliation. Preserve the existing identifier and
additional_identifier conditions, and keep the iOS/iPadOS sibling-title
regression test.
🪄 Autofix
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 Plus
Run ID: c86eecb7-4ae0-4a85-b28f-5521ebeb11e0
📒 Files selected for processing (2)
server/datastore/mysql/maintained_apps.goserver/datastore/mysql/maintained_apps_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/datastore/mysql/maintained_apps_test.go
getvictor
left a comment
There was a problem hiding this comment.
Done with review. The core changes are solid.
Resolves #50165 The hourly Fleet-maintained apps refresh normalized software names with four multi-table `UPDATE ... JOIN (<derived table>)` statements in one transaction. The derived tables need GROUP BY, so MySQL materializes them, and at scale the optimizer flips to scanning the target table once per materialized row. An UPDATE locks every row it reads -- the WHERE filter is applied after the row is locked -- so that plan held exclusive next-key locks on all of `software` and `software_titles` for the whole run. Inserts needing a shared lock on a `software_titles` row then blocked on the foreign key check, timing out the software install endpoints. Forcing the plan locally takes 657 next-key locks on a 651-row table while matching zero rows. The hazard is specific to UPDATE, so discovery still joins and only the writes changed: - Find mismatched rows with one SELECT per pass and table, join order pinned by STRAIGHT_JOIN so the catalog is the outer table and every software row is reached by index. A SELECT here is a non-locking consistent read. - Rename by primary key in batches of 500, each its own retried single-statement transaction, so locks are at most 500 record locks and release per statement. Every UPDATE re-checks `name <> ?`, keeping the pass idempotent. The comparison stays in SQL to preserve the column's utf8mb4_unicode_ci collation. - Move the pass to its own schedule (CronMacOSMaintainedAppNames) so a failed catalog fetch no longer skips it, with a back-dated first run so existing mismatches heal on upgrade. The refresh still triggers it on success, best effort, so a name change applies immediately. - Stop applying a macOS app's name to the iOS and iPadOS titles sharing its bundle identifier.
Each discovery SELECT in ReconcileMaintainedAppSoftwareNames now returns at most maintainedAppNameReconcileDiscoveryLimit rows, and each pass re-runs the query until a window comes back short. Renamed rows drop out of the next SELECT, so the loop walks the remainder without an offset. This bounds the pass's memory no matter how many rows are mismatched, instead of loading every mismatched (id, name) pair at once.
0384df9 to
9a578a6
Compare
… locking Cherry picks: #50226
Resolves #50165
The hourly Fleet-maintained apps refresh normalized software names with four multi-table
UPDATE ... JOIN (<derived table>)statements in one transaction. The derived tables need GROUP BY, so MySQL materializes them, and at scale the optimizer flips to scanning the target table once per materialized row. An UPDATE locks every row it reads -- the WHERE filter is applied after the row is locked -- so that plan held exclusive next-key locks on all ofsoftwareandsoftware_titlesfor the whole run. Inserts needing a shared lock on asoftware_titlesrow then blocked on the foreign key check, timing out the software install endpoints. Forcing the plan locally takes 657 next-key locks on a 651-row table while matching zero rows.The hazard is specific to UPDATE, so discovery still joins and only the writes changed:
name <> ?, keeping the pass idempotent. The comparison stays in SQL to preserve the column's utf8mb4_unicode_ci collation.Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit