fix(devloop): restart for a new Spring bean or entity the running app never had - #25595
fix(devloop): restart for a new Spring bean or entity the running app never had#25595totally-not-ai[bot] wants to merge 16 commits into
Conversation
Component scanning is a startup act, so a class annotated @component that did not exist when the application started gets no bean definition, and the first view to inject it fails with NoSuchBeanDefinitionException. The apply before it reported Stable, because every signal the runtime leg decides on is read from a loaded class and a new class has none: the connector had nothing to classify and blockedReason nothing to escalate on. The connector now reads the freshly compiled bytes of each requested class the JVM has not loaded - the same reading of the constant pool that declaresEntity does, and for the same reason - and reports a Spring stereotype found there as newBeans. The daemon escalates on that field and names it, so the restart is the loop's own answer rather than Spring's exception several steps later. Fixes #25559
…loaded Three things the first cut got wrong. A brand-new @entity fell through the same hole: classify() only ever sees a loaded class, so a type that appeared after startup was matched by no metamodel and reported live. The new bytes are read for it now, the way an @entity added to an existing class already was. "Not loaded in the app" turned out to be a race rather than a signal. HotswapAgent watches the output directory on its own schedule and defines a new class when it sees one, so whether the class is loaded by the time the reply is composed depends on which watcher got there first - and a defined class is still not a bean definition. Measured: the same apply reported Stable on one run and escalated on the next. So the connector now answers only what the bytes say (stereotypes=, for every requested class) and the daemon gates it on the inventory, which is re-seeded from disk at every registration and therefore does not flip. That also settles the case of a second apply --no-restart over the same new bean. The list of stereotypes grew by the two advice annotations, and the javadoc no longer claims the check only over-reports: a stereotype composed through a project's own meta-annotation is invisible to a constant-pool read, and that direction of error is worth stating where the trade is explained. The byte scan itself is now one method with the encoding comment, delegated to by declaresEntity and declaresSpringBean.
The coverage gate failed on new code, and it was pointing at something real: the change put its reasoning inside redefine(), which needs an Instrumentation handle and a live service, so none of it could be tested without the end-to-end module. Both halves of that method that are decision rather than effect are now their own package-private steps, and tested. inspect() reads what a request amounts to - which loaded copies to redefine, and what the classes and their new bytes say about whether a redefine can be the whole answer - and defines nothing. reply() renders the line the daemon parses, which nothing pinned before: it is read by field name, so a renamed or dropped field is a silently different answer rather than a parse failure, and the test asserts the whole line. The new field also moved to the end of that line, where adding it leaves every field a daemon already reads exactly where it was.
The classify half was only asserted in its all-empty form, which does not tell "it ran and found nothing" from "it was never called". A loaded Component is the one case of it that needs neither Spring nor JPA on the classpath, and ui= is load-bearing: a change-set with no Vaadin component in it is reported as live but not yet visible rather than simply stable.
tltv
left a comment
tltv
left a comment
There was a problem hiding this comment.
Rebase and fix conflicts. Fix SonarQ findings.
…ng-bean-restart # Conflicts: # vaadin-dev-server/src/test/java/com/vaadin/base/devserver/devloop/DevLoopRedefinerTest.java
…ists Both are SonarQube findings on this branch. typesUnknownToTheApp made ".java" a third literal in Compile, so it is a constant now and the two walks read it too. BEAN_DESCRIPTORS was declared between methods, next to the check that reads it; it moves to the constants at the top of the class, and ENTITY_DESCRIPTORS - which reads the same way and is used by the same method - goes with it rather than being left behind in the middle.
tltv
left a comment
tltv
left a comment
There was a problem hiding this comment.
Is the original issue reproducible with the main branch still with latest devloop fixes? Changes and new Spring beans could escalate restart already without this pull request changes.
@claude can you test that? |
|
Claude finished @tltv's task in 3m 47s —— View job Does #25559 still reproduce on
|
|
Restart will escalate if there's active browser opened on /bean view when |
Taking the new bean as a constructor parameter of a view that is already running - which is how the issue describes it - restarts on a stock JVM whatever the bean rule reports, because adding a parameter is a structural change and redefineClasses rejects it. Measured on main: that shape escalates with "class redefinition failed: attempted to add a method", and only a JVM with enhanced class redefinition shows the difference. The fixture is one new file for that reason, and now says so.
Parameterized rather than copied: the connector matches these by descriptor, one literal per annotation, so each is an entry that can be wrong on its own - and @component, which the rest are composed from, is the one a reader expects to see covered.
A stereotype is matched by exactly one literal, so a typo in one entry is one annotation that silently stops escalating while the rest keep working. All eight are asserted now, hand-spelled so the test cannot pass by reading the list it is checking - which is also where the per-entry guarantee belongs, since it costs no application. The integration test says so rather than claiming that job for itself.
tltv
left a comment
tltv
left a comment
There was a problem hiding this comment.
Reproduced a bug: added Extra.java with a bare @Component, ran apply, got classification: "hot-reload" / Stable — no restart.
What's actually going wrong: using the redefine diagnostic command against the new class, the app-side connector correctly reports stereotypes=Extra (it does see the @Component annotation on the never-before-loaded class). But the daemon still isn't escalating. Two live wrinkles I noticed along the way that point at where the remaining bug is:
notLoaded=0for a class that was never loaded before this transaction — HotswapAgent's own directory watcher is defining the new class into the JVM before the daemon'sREDEFINEround-trip runs. Thestereotypesfield is (correctly) computed from bytes so it survives that race, but thebeansfield (isSpringBeanon the now-loadedClassobject) also fires — it's just an annotation check, not a real Spring-context lookup, so it's a false "this is already a running bean" signal riding along. Doesn't explain the miss by itself, but it's noise worth being aware of.
Given the connector-side stereotypes value is right, the miss is almost certainly in Compile.typesUnknownToTheApp on the daemon side — whether it's really excluding "Extra" from its "unknown" set for this transaction (inventory/applied-map tracking), not in the annotation detection itself.
Reported: start, add a class with a bare @component, apply - and the first apply of a daemon's life reported hot-reload over a bean the context had no definition for. The connector's half was right (stereotypes=Extra, read from the bytes, so HotswapAgent defining the class first does not hide it); the daemon's half was not. The compile leg is built lazily, on the first apply, and seeds its inventory from disk as it is built. A source created between the application starting and that first apply is therefore in the inventory as though the application had always had it, so "missing from the inventory" answered no and nothing escalated. Every applied-once path hides it, which is why the existing tests - which all apply in setUp - passed. So the artifact answers too, and it is the fact that settles this case: a type whose .class was not on the classpath before this apply compiled it is one the application had nothing to load, nothing to scan and nothing to register. That has to be read before javac writes anything, so the question moves back to the detection phase where the change-set is built. An edit to a class the application does have keeps its artifact and its inventory entry, so it still hot swaps.
|
@tltv Reproduced it, and your diagnosis was exactly right — the miss was in What was wrong. The compile leg is built lazily, on the first apply, and seeds its inventory from disk as it is built. So a source created between the application starting and that first apply lands in the inventory as though the application had always had it, and after: The fix. The inventory is no longer asked on its own. A second, independent fact settles this case: a type whose Why every existing test missed it: they all apply once in On your two wrinkles — both accurate, and worth recording:
|
…apply The baseline is meant to be "what the running application started with", and it was read when the compile leg was first needed instead - which is the first apply, by which time the disk has moved on. Anything added in between was recorded as something the application had always had. Two misses came out of that, both on the first apply of a daemon's life. A new @component reported hot-reload, which is the reported one. Worse, if something other than the daemon had compiled the class first - an IDE building on save, or a plain mvn run - the apply reported no changes at all: the artifact was newer than the source and the inventory had the stamp, so nothing in the change-set saw the file. Registration is when that question has an answer, so the baseline is taken there. Only from a classpath that is already resolved: it runs on the thread answering the registration connection, which must not wait on Maven, and a project mid-resolve simply leaves the baseline to the first apply as before. Measured, first apply after start, with the class compiled by something else beforehand: was "no changes", now "new Spring bean (Extra)".
…lock Taking the baseline when the application registers was the right moment and not a reliable one: the command that waited for the registration returns before the daemon has handled it, so a file created immediately afterwards still landed inside the baseline. The integration test for the ordering caught it - it passed alone and failed in the suite. So the baseline no longer trusts when it happens to be taken. A source is recorded as the application's own only if it was on disk before the application was launched, and what separates a new type from an edited one is then the class file: a type whose .class was not on the classpath at launch is one the application had nothing to load, while an edited type's class was there all along. Both sides are read against the launch timestamp, which does not move. Also from the review of the previous commit: the registration seed no longer walks the tree twice, it keeps the frontend cutoff that leaves a file edited during startup visible to the first apply, its one diagnostic line goes to daemon.log instead of a no-op sink, and a classpath that fell back to the application module alone no longer counts as resolved - building a baseline on that would report "module set changed" with no pom edit behind it.
There was a problem hiding this comment.
Found three issues:
-
[P1] Compile.java:844 treats a known source file as proof that every class it declares existed at startup. Adding a nested static
@Component, or another package-private top-level component, to an existing source therefore produces nounknownTypesentry. The connector reports the new class’s stereotype, butblockedReasondoes not restart, leaving the bean unregistered—the core failure this PR intends to fix. -
[P1] Compile.java:870 updates the same
appliedmap used as the startup inventory after successful hot swaps. Sequence: add a plain class → apply successfully → annotate it with@Component→ apply again. The first apply inserts its source intoapplied; the second considers the class known even though component scanning has never seen it, so no restart occurs. -
[P2] Compile.java:814 and TransactionEngine.java:1662 correlate classes by simple name. If an existing stereotype and a newly added plain class share a name in different packages, editing both in one transaction falsely reports the plain class as a new Spring bean and forces an unnecessary restart. Binary names are needed to preserve identity.
Also take care of the remaining 2 Sonar findings.
There was a problem hiding this comment.
@tltv All three fixed, with a test each, and the two Sonar findings with them.
P1, per-source answer. Right — and the fix is to stop asking about sources. The question is now asked of each class the apply compiled, against the class files its output packages held before it compiled them: a class file that was not there, or that is newer than the launch, is one the application started without. A second top-level @Component in a file the application has always had now escalates under its own name.
One thing worth recording from testing it: a nested @Component escalates on this JVM for an unrelated reason — nesting rewrites the enclosing class's NestMembers attribute and redefineClasses refuses outright (class redefinition failed: attempted to change the class NestHost, NestMembers...). So the test uses a second top-level class, which leaves the enclosing class byte-identical and actually exercises this rule rather than the JVM's refusal. The nested case is covered too, by the same per-class read.
P1, applied doubling as the startup inventory. Same root, same fix: the class file an earlier apply wrote is newer than the launch, so add-plain-class → apply → annotate → apply now escalates on the second apply. There is an integration test for exactly that sequence.
P2, simple-name correlation. Agreed, it was an identity bug. stereotypes= now carries binary names — alone among the reply's fields, because it is the one read by machine rather than by a person, and the daemon matches the change-set against it exactly. Only the reason shortens the name again, so the message still reads new Spring bean (Extra). The unit test pins that a stereotype sharing a simple name with a new class in another package escalates nothing.
Sonar. The class-file read no longer swallows an IOException: an unreadable output directory is not a state to decide a verdict from, so the apply fails with that as its reason. And the protocol's free-text tail is a constant.
Green: 149 daemon and 346 vaadin-dev-server unit tests, and 38 dev-loop ITs — the exception being DevLoopBrowserIT.siblingModuleEdit_isVisibleOnceTheViewRendersAgain, which times out on my machine identically at the commit before any of this work, so it is not from these changes.
Re-measured end to end on the reported flow (start, add a class with a bare @Component, apply) and on the variant where something else compiles the class first: both tx#1 classification=restart, escalating with the named reason.
… simple name Three ways the answer could be wrong, all of them reported. A source was taken as proof about every class it declares. Add a second top-level @component to a file the application has always had, or a nested one, and the source is in the baseline, so the class beside it - which the context has never scanned - was reported as the application's own. The question is now asked of each class the apply compiled, against the class files its output packages held beforehand: one that was not there, or that is newer than the launch, is one the application started without. Which also settles the second case, add-then-annotate: the class file an earlier apply wrote is newer than the launch, so annotating it on a later apply still escalates, where the source inventory had already claimed it. And the correlation was on simple names, so a new plain class could be reported as a bean because an unrelated stereotype in another package shares its name - a restart nobody needed. The connector reports that one field under binary names now, alone among its fields, because it is the one read by machine rather than by a person; the reason still prints the short name. Also the two SonarQube findings: the class-file read no longer swallows an IOException - an unreadable output directory is not a state to decide a verdict from, so it fails the apply with that as the reason - and the protocol's free-text tail is a constant.
…files Dating them broke the thing this branch exists to protect. An apply rewrites the class files it swaps, so the second method-body edit to a @service read its own class as newer than the launch, called it a bean the context had never scanned, and restarted for it. Measured on the fixture app, and there is an integration test for it now: a bean edited twice over has to hot swap both times. So the classpath is snapshotted by binary name when the baseline is taken - once per application start, beside the source walk that was already there - and the question is a set lookup against it. A class the launch did not have stays unknown however many applies rewrite it, which is the add-then- annotate case, and a class it did have stays its own however often it is recompiled, which is the hot swap. Nothing depends on a timestamp, so nothing depends on file granularity or on the clock. It also drops the ordering constraint that the snapshot had to be read before javac ran, and with it the failure branch that turned a concurrent IDE build into a failed apply: the walk tolerates an unreadable output tree the way every other walk here does.
|
1 participant
Summary
When you add a new Spring bean and run
apply, the dev loop said "hot-reload" and the app then failed with Spring'sNoSuchBeanDefinitionException. Component scanning only runs at startup, so a brand-new bean needs a restart. The dev loop now spots those classes and restarts instead.What changed
Behavior change:
applynow escalates to a restart, with the reasonnew Spring bean (X), when the change-set contains a class carrying@Component,@Service,@Repository,@Controller,@RestController,@ControllerAdvice,@RestControllerAdviceor@Configurationand the running application never had that class. This only affects dev-loop users; before, the same change was reported as a successful hot reload. Nothing else about apply changes: a bean the app started with still hot swaps as before.The answer is built from two halves, because neither side can give it alone:
DevLoopRedefiner) reads the stereotype off the compiled bytes, not off the loaded class — HotswapAgent may have defined the class already, and a defined class is still not a bean definition. It reports this in a newstereotypes=field on theREDEFINEreply, appended last so every field an existing daemon reads stays where it was. Unknown fields are ignored on both sides, so this is compatible in either direction; an old connector simply sends no field and nothing escalates.Compile) answers "did the running application have this class?" from a snapshot of the classpath taken at each application start, keyed by binary name. No timestamps are involved, so nothing depends on file-time granularity or on the clock.Three correctness details fall out of that:
Supporting fixes:
Launch.projectIfResolved) so the registration thread never waits on Maven. A project mid-resolve falls back to the old behaviour.DevLoopRedefiner.redefinewas split:inspect(decide) andreply(format) are now separate and testable without a running application.flow-devloop-daemon/README.mddocuments the new escalation and its one known gap.No public or protected API changed — every touched class is package-private.
Test summary
new Spring bean (X), even though nothing was redefinedNoSuchBeanDefinitionExceptionnew Spring beanmvncompiled it — and never reports "no changes"inspectreports duplicates, not-loaded names and UI classes; a not-loaded class off the search path is no error; a loaded class with missing bytes is the one errorOK ...reply line, includingstereotypes=lastprojectIfResolvedreturns empty until a classpath has been resolvedTests added or changed on this branch:
DevLoopApplyIT.aBeanEditedTwiceOverStaysAHotSwapBothTimes— 2DevLoopRestartIT.aNewSpringBean_escalatesEvenThoughNothingWasRedefined(@Component,@Service) — 1DevLoopRestartIT.aSecondClassInAFileTheAppAlreadyHas_stillEscalates— 4DevLoopRestartIT.aClassAnnotatedAfterAnEarlierApply_stillEscalates— 5DevLoopRestartIT.aNewSpringBean_escalatesOnTheFirstApplyOfADaemonsLife(daemon-compiled and externally compiled) — 6TransactionEngineTest.blockedReason_escalatesForABeanTheRunningApplicationHasNeverHad— 1, 2, 3CompileTest.classesUnknownToTheApp_answersFromTheLaunchSnapshot— 2, 4, 5DevLoopRedefinerTest.declaresFromBytes_answersForAClassNothingHasLoaded— 7DevLoopRedefinerTest.inspect_readsALoadedClassAndTheBytesItIsAboutToBeGiven,inspect_namesTheBeansAndEntitiesThisJvmHasNoClassFor,inspect_aLoadedClassWithNoNewBytesIsTheOneError— 8DevLoopRedefinerTest.reply_carriesEveryFieldTheDaemonReadsAVerdictFrom— 9LaunchTest.projectIfResolved_isEmptyUntilOneHasBeenResolved— 10Left untested on purpose: a stereotype composed through a project's own meta-annotation, which only names the custom annotation in its constant pool — a known limitation written down in the daemon README, not a behaviour this branch claims.