Repository navigation
fix(reporting): serialise SQS schema updates of concurrent report calculations - #274
Conversation
…culations Report calculations now run on a thread pool with two threads, and each updates the SQS schema before its queries. After a config change, two of them could both find the schema outdated and write it, and the second write failed with a CouchDB 409, failing its calculation. updateSchema is now synchronized, and a remaining 409 (from another writer) is accepted when the schema it finds is the one it meant to write. Fixes #273 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fb2hmo8XQokkndtteXpFpC
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Serialize updates and handle conflicts application/aam-backend-service/src/main/kotlin/com/aamdigital/aambackendservice/reporting/report/sqs/SqsSchemaService.kt |
updateSchema() is synchronized. A CouchDB conflict is suppressed only when the stored schema’s configVersion matches the proposed schema. Other errors and mismatched versions are rethrown. |
Validate concurrent updates and conflict outcomes application/aam-backend-service/src/test/kotlin/com/aamdigital/aambackendservice/reporting/report/sqs/SqsSchemaServiceTest.kt |
Tests verify concurrent update behavior and check that matching versions suppress conflicts while differing versions rethrow them. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested reviewers: tomwwinter, abhinegi2
Merge Risk: 
5a5cd
-
The change serializes schema updates within one instance and accepts harmless conflicts when the stored schema already matches. No new merge-blocking risk was established; the cross-instance overwrite race predates this PR.
Pre-merge checks: 
-
Check name Status Explanation Resolution Linked Issues check Issue #273has two coding objectives that this PR meets and one that it does not.
Met: (1) Prevent the 409 in updateSchema. SqsSchemaService.updateSchema() is annotated@Synchronized, so concurrent r…Implement objective (3) in this PR, or narrow the link. Option A: change the report-calculation failure logging so the exception message is stable (no calculation id in the message), and pass the calculation id as a tag or extra value. Opti… -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(reporting): serialise SQS schema updates of concurrent report calculations" matches the main change. The summary shows updateSchema() is now synchronized, and a CouchDB conflict is supp… Out of Scope Changes check All production changes are in SqsSchemaService.kt and serve issue #273. The@Synchronizedannotation, the 409 handling with configVersion comparison, and the extracted private fetchCurrentSchema() hel…Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Title check
-
Explanation
The title "fix(reporting): serialise SQS schema updates of concurrent report calculations" matches the main change. The summary shows updateSchema() is now synchronized, and a CouchDB conflict is suppressed only when the stored configVersion matches the schema being written. The tests cover concurrent updates and both conflict outcomes. The title is short, specific, and uses a conventional commit prefix. It does not cover the conflict-acceptance logic, but that is expected for a concise title.
Full details: Out of Scope Changes check
-
Explanation
All production changes are in SqsSchemaService.kt and serve issue
#273. The@Synchronizedannotation, the 409 handling with configVersion comparison, and the extracted private fetchCurrentSchema() helper are all used by the fix. The added imports (DefaultCouchDbClientErrorCode, ExternalSystemException) are needed for the 409 check. The test changes in SqsSchemaServiceTest.kt add regression cases and split stubs (stubConfigDocument, stubSchemaDocument) to support them. No unrelated files or behaviour changes were found in the reviewed diffs. The diffs for SqsSchemaService.kt and SqsSchemaServiceTest.kt were read at the reviewed head against the merge base. Other changed files, if any, were not inspected; the change summary lists only these two files.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
-
Full details: Linked Issues check
-
Explanation
Issue
#273has two coding objectives that this PR meets and one that it does not.Met: (1) Prevent the 409 in updateSchema. SqsSchemaService.updateSchema() is annotated
@Synchronized, so concurrent report calculations in one process serialise the read-check-write. A 409 that still occurs is accepted only when fetchCurrentSchema().configVersion equals the intended schema's configVersion; otherwise the ExternalSystemException is rethrown. This matches the issue's proposed fix (tolerate 409 or serialise updates). (2) Regression test. SqsSchemaServiceTest adds three tests: concurrent calls write once (CyclicBarrier-based, fails without the lock), a 409 with the same schema is accepted, and a 409 with a different schema is rethrown. The test diff was read and confirmed.Not met: (3) Group by cause. The issue asks that the exception message stop including the calculation id, with a stable message and the id moved to tags or extra. The code diff does not change the log or exception message. The PR description states that Sentry grouping is unchanged. This is a concrete coding item in the issue's proposed fix, and it is not implemented.
Resolution
Implement objective (3) in this PR, or narrow the link. Option A: change the report-calculation failure logging so the exception message is stable (no calculation id in the message), and pass the calculation id as a tag or extra value. Option B: change the PR reference from "Fixes
#273" to a non-closing reference, and record the grouping change as a separate follow-up issue, so#273is not closed with objective (3) open.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Fixes #273
Cause
Since RabbitMQ was retired, report calculations run on a pool of two threads (
CALCULATION_CONCURRENCY = 2), and each one callsSqsSchemaService.updateSchema()before its queries. When the schema is out of date (a new system, or a changed entity config), both calculations find it outdated and both write it. CouchDB rejects the second write with a 409, and that calculation fails.Changes
updateSchema()is now@Synchronized, so the second calculation finds the schema already up to date and does not write it.Notes
updateSchemais not a data migration. It keeps the SQS design document_design/sqlite:configin sync with the entity config, which SQS needs to run any query. The frontend never writes that document, so this service is its only writer.Testing
SqsSchemaServiceTest: all 8 tests pass;ktlintCheckpasses.Generated by Claude Code
Summary by CodeRabbit