fix(lifecycle): clear storage before services clear on state reset - #3388
Conversation
performReset ran every Resettable.clear() first and storageFactory .clearAll() last. Services that re-create bootstrap state in clear() lost it again right away. IAM Identity Center is one: after a /_floci/state/reset its bootstrap identity store was gone, so every SCIM call answered 401 until a restart. This is what broke ScimIntegrationTest in CI shard 3 since floci-io#3331. EmulatorInfoControllerIntegrationTest runs in the same test application before it and posts the reset endpoints. Clear storage first, then let services clear and re-seed. The clear() implementations that iterate state to stop timers or sockets read in-memory maps, not storage, so the order change does not affect them.
There was a problem hiding this comment.
🟡 Changes recommended
Reset can seed bootstrap state in the wrong account partition, leaving default-account SCIM requests unauthorized.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes lifecycle reset ordering so services can recreate IAM Identity Center bootstrap state after storage is cleared.
Changes:
- Clears storage before invoking resettable services.
- Adds integration coverage for SCIM access after reset.
File summaries
| File | Summary |
|---|---|
src/test/java/io/github/hectorvent/floci/lifecycle/EmulatorInfoControllerIntegrationTest.java |
Adds regression coverage for bootstrap state surviving reset. |
src/main/java/io/github/hectorvent/floci/lifecycle/EmulatorInfoController.java |
Reorders reset operations; bootstrap reseeding must use explicit account-aware storage methods to avoid seeding the caller’s partition. |
Review details
Suppressed comments (1)
src/main/java/io/github/hectorvent/floci/lifecycle/EmulatorInfoController.java:151
clearAll()flushes every backend before this loop, butclear()can write bootstrap data afterward. In the supportedhybridmode those writes only mark the backend dirty, so an immediate restart can load the empty snapshot produced by the reset and lose any state re-seeded by a service. Flush once after allResettable.clear()calls so the reset persists the final in-memory state.
storageFactory.clearAll();
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| storageFactory.clearAll(); | ||
| for (Resettable r : resettables) { | ||
| r.clear(); |
There was a problem hiding this comment.
I think the mechanism is right, the failure is not.
hasIdentityStore() falls back to scanAllAccounts(), so a bootstrap instance seeded under another account's partition is still found.
Checked on a JVM build of this branch. A reset signed as 111111111111 followed by a default-account SCIM ServiceProviderConfig call returns 200, not 401.
ListInstances for the default account re-seeds its own partition on first use, and the misplaced entry is not visible to ListInstances of either account.
This is pre-existing behavior of ensureBootstrapInstance() and independent of the order change in this PR.
Using getForAccount/putForAccount there is a reasonable hardening, but it belongs in its own change.
Let's keep this PR small.
pgermosen
left a comment
There was a problem hiding this comment.
This is the real fix. Confirmed the ordering bug directly, clear() running before storageFactory.clearAll() meant anything a service re-seeded in its own clear() got wiped right back out.
Also went through Copilot's account-partition concern myself: get/put on AccountAwareStorageBackend do resolve through the ambient request context rather than the ownerAccountId argument, so the finding is technically real. But the empirical test against a JVM build settles it, hasIdentityStore's scanAllAccounts fallback and listInstances' self-heal both cover it, so the actual symptom this PR exists to fix is not affected. Agreed that the getForAccount/putForAccount hardening belongs in its own change rather than growing this one.
Regression test hits exactly the sequence that broke. Approving.
Summary
Note
#3387 fixed the symptom for Identity Center. The cause is still in
performReset()in which everyResettable.clear()runs beforestorageFactory.clearAll(). So anything a service re-seeds inclear()is wiped right after.This PR fixes that order, so future services with default state do not hit the same bug, and adds a regression test on the reset endpoint. Both fix can coexist.
Fixes #3385
performReset()ran everyResettable.clear()first andstorageFactory.clearAll()last, so any service that recreates its bootstrap state inclear()lost it again until the next restart. Today onlySsoAdminServicedoes this, which is why SCIM answered 401 after a reset, but every future service with default state would hit the same bug. Wiping storage first and letting services clear and re-seed afterwards closes the trap for all of them.This is what broke
ScimIntegrationTestin CI shard 3 since #3331.EmulatorInfoControllerIntegrationTestruns in the same test application before it and posts the reset endpoints.Clear storage first, then let services clear and re-seed. The
clear()implementations that iterate state to stop timers or sockets read in-memory maps, not storage, so the order change does not affect them.Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
No change.
Checklist
./mvnw testpasses locally