fix: a reveal and the access-log writer no longer wait for each other (release/0.1 port of #45) #46
Open
james.maes
wants to merge 1 commits from
fix/reveal-access-log-deadlock-0.1 into release/0.1
pull from: fix/reveal-access-log-deadlock-0.1
merge into: QRun-IO:release/0.1
QRun-IO:develop
QRun-IO:fix/reveal-access-log-deadlock
QRun-IO:release/0.1
QRun-IO:fix/reentrant-rule-reload-0.1
QRun-IO:fix/reentrant-rule-reload
QRun-IO:merge/develop-into-release-0.1-rc6
QRun-IO:perf/request-scoped-resolver-and-cached-rules
QRun-IO:main
QRun-IO:dependabot/maven/develop/maven-minor-and-patch-47b49de09a
QRun-IO:dependabot/github_actions/develop/QRun-IO/qqq-orb/dot-github/workflows/qbit-codeql.yml-1c389210e6dfda1630d053c07ae4dae523dd5576
QRun-IO:dependabot/github_actions/develop/QRun-IO/qqq-orb/dot-github/workflows/qbit-release-rc.yml-1c389210e6dfda1630d053c07ae4dae523dd5576
QRun-IO:feature/orb-0.8-track
QRun-IO:feature/orb-0.7.18
QRun-IO:feature/orb-0.7.17
QRun-IO:feature/orb-standardize-release-0.1
QRun-IO:feature/orb-main-standardization
QRun-IO:feature/shared-codeql
QRun-IO:feature/orb-0712-stable-gate
QRun-IO:feature/orb-rc-worker
QRun-IO:feature/orb-0.7.11
QRun-IO:feature/central-ci
QRun-IO:feature/access-log-lifecycle
QRun-IO:feature/virtual-fields-and-idle-flush
QRun-IO:dependabot/maven/develop/com.puppycrawl.tools-checkstyle-14.3.0
QRun-IO:ci/release-rc-secrets-inherit
QRun-IO:feature/ui-api-1.6
QRun-IO:feature/remove-standins
QRun-IO:feature/orb-0.6.9
QRun-IO:feature/retention-and-wipe
QRun-IO:feature/app-home
QRun-IO:feature/reveal-and-break-glass
QRun-IO:feature/policy-and-access-log
QRun-IO:fix/pin-qqq-rc
QRun-IO:feature/scaffold
No Reviewers
Labels
Clear labels
architecture
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
java
question
secuurity
wontfix
Architecture review: coupling, cohesion, UI API
Something isn't working
Improvements or additions to documentation
This issue or pull request already exists
New feature or request
Pull requests that update GitHub Actions code
Good for newcomers
Extra attention is needed
This doesn't seem right
Pull requests that update java code
Further information is requested
This will not be worked on
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: QRun-IO/qbit-secured-data#46
Reference in new issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Port of #45 to the 0.1 release line: the same commit, cherry-picked onto
release/0.1(the two branches differ only in the version inpom.xml). It is a pull request of its own, like #44, so that it can be reviewed and released without waiting for a merge of develop. Merge #45 first.The defect
A reveal can deadlock with the access-log writer, and then the node stops answering: every later request blocks, because every request asks this QBit for a classification.
RevealService.revealandBreakGlassService.start,endandexpirehold the runtime's monitor for their whole operation.classificationRows()andactiveRetentionRules()were synchronized on the same monitor, and core calls them for every field it decides on and for every filter criterion of every query, including the queries made inside this QBit's own writes. The access-log writer holds the anomaly counters while it inserts a row, and that insert asks for a classification; a reveal inserts its own access-log row:Seen in a host on PostgreSQL with 0.1.0-RC.7, when a person read a record with a masked field and revealed it straight away (thread dump in #45). The same structure is in 0.1.0-RC.5 and RC.6.
The fix
The rule rows a node keeps (classifications, active retention rules) have a lock of their own, held only while they are answered or read. A read is a query made as the system and takes none of the QBit's other locks, so nothing that holds the rule lock waits for one. The runtime's monitor is left to serialising reveals and break-glass changes.
Tests
SecuredDataLockOrderTest, on H2 in PostgreSQL mode with the audit tables: a thread is parked inside a reveal, and inside the start of a break-glass session, holding the runtime's monitor; the access-log writer must drain its queue and another thread must be told a classification and the active retention rules meanwhile; then the parked operation must finish. Both tests fail without the fix.Verification
./mvnw verifyon this head (c2fde97): 114 tests, 0 failures, 0 Checkstyle violations, coverage checks met, BUILD SUCCESS.🤖 Generated with Claude Code
A reveal, and the start, end and expiry of a break-glass session, hold the runtime's monitor from their first check to their last write. Asking for a classification (SecuredDataRuntime.classificationRows, and activeRetentionRules) took the same monitor, and every thread asks: core asks for every field it decides on and for every filter criterion of every query, including the queries made inside this QBit's own writes. So a thread that asked while holding another lock of this QBit could not be answered during a reveal, and if the reveal needed that lock next, both waited for ever. The access-log writer is such a thread. It holds the anomaly counters while it inserts a row; the insert counts earlier rows and writes an audit, both queries with criteria. A reveal inserts its own access-log row. reveal: runtime's monitor, then the anomaly counters writer: the anomaly counters, then the runtime's monitor Seen in a host on PostgreSQL: a person read a record with a masked field and revealed it at once; the writer's tick drained the read's log rows at the same moment. The request and the writer deadlocked (thread dump: the request in SystemWrites.insertOne under RevealService.reveal, the writer in SecuredDataRuntime.classificationRows under AuditAction.fetchIdFromName under its own insert), and every later request blocked in classificationRows. The same order existed between a reveal and the resolver's monitor, and the key source's. - The rule rows have a lock of their own (ruleLock), held only while they are answered or read. A read is a query made as the system and takes none of the QBit's other locks, so nothing that holds ruleLock waits for one. - The runtime's monitor is left to what it was for: one reveal or break-glass change at a time. Tests (SecuredDataLockOrderTest, on H2 in PostgreSQL mode with the audit tables): a thread is parked inside a reveal, and inside the start of a break-glass session, holding the runtime's monitor; the access-log writer must drain its queue and another thread must be told a classification and the active retention rules meanwhile, and then the parked operation must finish. Both tests fail without the fix (neither thread finishes). Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.