janhoy commented on PR #5047:
URL: https://github.com/apache/solr/pull/5047#issuecomment-6066798920
🤖 Bug fix and pr comment below written by Claude Fable
---
### Flaky `PackageToolPicocliTest.testPackageTool` and the fix
Running `org.apache.solr.cli.*` on this branch, `PackageToolPicocliTest`
failed in roughly a third of runs while `PackageToolTest` always passed. With
`-Ptests.seed=CE8CB19A405A7699` the picocli test failed 7 of 8 runs. The
failing step is the second `install question-answer` in the random "auto-update
to latest" branch of `testPackageTool`, which exits 1 after `Actual: 1.0.0,
expected: 1.1.0` / `Failed verification after deployment`.
This is not a bug in the port. The commons-cli `PackageTool.install()` was
`void`, so the legacy path printed `installation failed` and still exited 0,
which hid a pre-existing server-side race. The new `PackageInstall` correctly
propagates the result, which is why the test now sees it.
**The race.** `RepositoryManager.install` adds the new version through `POST
/api/cluster/package {add: ...}` and then immediately verifies the collections
pegged to `$LATEST`. On the server, `PackageAPI.Edit.add` writes
`packages.json` and calls `notifyAllNodesToSync(znodeVersion)`, and each node
answers via `Read.syncToVersion`. That method considered a node synced as soon
as `pkgs.znodeVersion` had reached the expected version. But that field is
bumped by the node's own ZK watcher thread at the start of `refreshPackages`,
before `SolrPackageLoader.refreshPackageConf()` has built the new version's
classloader and reloaded the plugins. So when the watcher fires first, which is
the common case, the sync request returns instantly while the reload is still
in flight, `add` returns to the client, and the verify reads the old plugin
version. The explicit `refresh` command had the same hole: `notifyListeners`
would run against a `SolrPackage` whose version list the watcher thread had n
ot finished updating, so it reloaded nothing. I first tried posting a
`refresh` from the client before verifying, the way the deploy path does, and
it failed deterministically for exactly that reason.
**The fix** (separate commit on this branch, `3ca62e67c71`) is server-side
and small:
- `SolrPackageLoader.refreshPackageConf()` and `notifyListeners()` are now
`synchronized`, so a sync or refresh request blocks until any application of
`packages.json` already started by the ZK watcher has completed, and is a no-op
afterwards.
- `PackageAPI.Read.syncToVersion` always goes through `refreshPackageConf()`
once the expected version is visible, instead of skipping it when the watcher
had already bumped the version number.
Lock order stays one-directional (loader monitor, then `PackageListeners`,
then `PackagePluginHolder`), and nothing on the listener path calls back into
the loader, so there is no new deadlock risk.
**Verification.** The failing seed now passes 8 of 8 runs, both
`PackageToolTest` and `PackageToolPicocliTest` pass on repeated random seeds,
and `org.apache.solr.pkg.*`, `org.apache.solr.filestore.*` and
`TestContainerPlugin` all pass. `gradlew check -x test` is green.
A second small commit (`7ee9561c11c`) tidies the test:
`testDeployValidationMessages` created `validation-test` on `conf1`, the same
configset as `abc`, so after a deploy to `abc` the package also showed as
deployed on `validation-test` via the shared `PKG_VERSIONS` params. It now gets
its own configset so the two test methods do not influence each other. Both
twins pass on repeated runs and `list-deployed` reports only `abc`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
