6.3 KiB

Raw Blame History

W3/T6 Review: Permissions, Error Codes, Concurrency, and Privacy

Verdict

Base: a5730a7
Head: ca85ab5
Diff: 6 files, +936/-3
Spec compliance: ❌ Issues found
Task quality: Needs fixes
Blocking findings: 1 Important

Scope Reviewed

Read the T6 brief, execution contract, checklist and company-knowledge specs, design decisions, reviewer guidance, production diff, and all three added integration-test classes.

Verified the explicit /onboarding/** authenticated matcher, STUDENT/COMPANY service role checks, company ownership predicates, checklist HTTP response whitelists, privacy assertions, optimistic-version mappings, and the placement of pessimistic locks relative to transaction reads.

Evidence

Default focused suite:

cd D:\1\zhi\zhilai\mydreamjob-server
$env:JAVA_HOME='D:\java'
$env:Path='D:\java\bin;'+$env:Path
& 'D:\maven\apache-maven-3.9.7\bin\mvn.cmd' '-Dtest=OnboardingAuthorizationIntegrationTest,OnboardingConcurrencyIntegrationTest,OnboardingPrivacyTest' test

Result:

Tests run: 19, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Production-isolation regression:

& 'D:\maven\apache-maven-3.9.7\bin\mvn.cmd' -q '-Dtest=OnboardingConcurrencyIntegrationTest' '-Dspring.datasource.hikari.transaction-isolation=TRANSACTION_REPEATABLE_READ' test

Result:

Tests run: 10, Failures: 5, Errors: 0, Skipped: 0

The focused 49→50 test failed because both concurrent creates succeeded:

concurrentCreatesAtLimitBoundaryNeverWriteFiftyFirstItem
Expected size: 1 but was: 2

git diff --check a5730a7..ca85ab5 passed. No implementation files were modified during review.

Strengths

SecurityConfig.java:130 explicitly marks /onboarding/** as authenticated; anonymous requests return 401 through the configured entry point.
Student and company role checks are enforced in service methods, not only by token claims. Company ownership is checked through owner_user_id before item mutation.
Cross-company update/delete paths return 9701 without returning the target item or modifying it.
OnboardingChecklistService.java:554 maps both optimistic-lock and duplicate-insert failures for progress to 9705; CompanyKnowledgeService.java:138 similarly maps knowledge conflicts to 9706.
Soft-delete preserves history and student effective-checklist queries filter inactive items.
Privacy tests use response JSON for several write/read/error paths and assert removal of ownerUserId, studentUserId, hashes, tokens, and moderation internals.

Issues

Critical

None.

Important

The 50-active-item guard is not transaction-safe under the production database's default isolation. OnboardingChecklistService.java:153 calls requireCompanyOwner(), whose normal read at OnboardingChecklistService.java:256 establishes the transaction snapshot before lockCompany() runs at OnboardingChecklistService.java:155. The subsequent findAll() at OnboardingChecklistService.java:168 and active count at OnboardingChecklistService.java:182 are ordinary consistent reads, so a waiting writer can still observe the pre-commit snapshot. With 49 active rows, two concurrent creates can both observe 49, both pass the check, and both insert, producing 51 active items. MySQL defaults to REPEATABLE READ and application.yml does not override it. The H2 test at OnboardingConcurrencyIntegrationTest.java:40-41 also does not set the production isolation level, which is why the default run masks the defect.

Reproduction command:
```
mvn -q '-Dtest=OnboardingConcurrencyIntegrationTest' '-Dspring.datasource.hikari.transaction-isolation=TRANSACTION_REPEATABLE_READ' test
```
The test fails at OnboardingConcurrencyIntegrationTest.java:111 with two successful CompanyChecklistItemView results instead of one success and one 9703 conflict; the seeded 49 rows plus both inserts means the database has 51 active items.

Required fix: make the post-lock idempotency and active-count decisions from locking/current reads, or explicitly run these write transactions at READ_COMMITTED. A company-row PESSIMISTIC_WRITE lock alone is insufficient when the transaction snapshot was established by a pre-lock ordinary read. Add a regression test that runs the 49→50 boundary under the same isolation model as MySQL or against MySQL 8.4.

Minor

Several concurrency tests over-constrain outcomes beyond the T6 contract. OnboardingConcurrencyIntegrationTest.java:75 requires both same-key callers to succeed, while the task brief permits 9704; OnboardingConcurrencyIntegrationTest.java:184 and OnboardingConcurrencyIntegrationTest.java:202 similarly require all progress/knowledge callers to succeed even though the contract permits deterministic 9705/9706 conflict responses. Under TRANSACTION_REPEATABLE_READ, those tests fail for contract-permitted outcomes. Assert exactly one successful mutation and one allowed conflict/replay, and reject unexpected exception types.
CompanyKnowledgeService.java:135-140 only translates optimistic-lock and integrity exceptions. Under the repeatable-read regression run, the same-version knowledge test surfaced an unmapped H2 transaction-rollback/deadlock exception and failed the 9706 assertion at OnboardingConcurrencyIntegrationTest.java:238-241. Verify and, if reproducible on MySQL, map lock acquisition/lock-timeout failures at the service boundary to the appropriate 9702/9705/9706 contract error.

Cannot Verify From Diff

Real MySQL 8.4 execution was not available in this review. The repeatable-read H2 reproduction is sufficient to reject the current locking design, but the final B3 gate must also run the real MySQL path and assert 50 active items after concurrent boundary creation.

Assessment

The permission, ownership, privacy, and default-isolation tests are well structured, and the focused suite is green. The implementation is not ready to accept because its central concurrency guarantee can be violated under the production MySQL default isolation: concurrent creates can write the 51st active checklist item.

Receipt

ssf execution review changes/n04-onboard --wave W3 --base a5730a7 --head ca85ab5 --report changes/n04-onboard/.superpowers/sdd/reviews/W3-T6.md --verdict fail

6.3 KiB Raw Blame History