Race condition between cosmovisor and upgrade handler

[iramiller]

Summary of Bug

Under certain conditions the cosmosvisor process can terminate the blockchain executable before an upgrade plan file is flushed to disk. This action prevents a successful upgrade due to a missing upgrade info file.

Version

Tested against 0.42.2
Issue is present in latest/master

Steps to Reproduce

Create a local instance of the network running with cosmovisor. Use a local binary for upgrade such that there is no latency from downloading a binary. The cosmosvisor will terminiate the process as soon as the log message is received but before the upgrade info file can be persisted to disk.

In the following code the upgrade required message is written to the log on line 45 while the upgrade file is dumped on 49.

cosmos-sdk/x/upgrade/abci.go

Lines 45 to 52 in a78f777

	ctx.Logger().Error(upgradeMsg)
	// Write the upgrade info to disk. The UpgradeStoreLoader uses this info to perform or skip
	// store migrations.
	err := k.DumpUpgradeInfoToDisk(ctx.BlockHeight(), plan.Name)
	if err != nil {
	panic(fmt.Errorf("unable to write upgrade info to filesystem: %s", err.Error()))
	}

Meanwhile in the cosmovisor process the monitor will execute an unclean process termination to force an immediate exist when the message appears in the logs

cosmos-sdk/cosmovisor/process.go

Lines 118 to 124 in a78f777

	upgrade, err := WaitForUpdate(scan)
	if err != nil {
	res.SetError(err)
	} else if upgrade != nil {
	res.SetUpgrade(upgrade)
	// now we need to kill the process
	_ = cmd.Process.Kill()

Remediation

Move the log message on abci.goL45 after the k.DumpUpgradeInfoToDisk on line 49.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[aaronc]

We are actually going to switch cosmovisor to just wait for the file instead of reading stdout. That should address this. Any update on the status of that fix @robert-zaremba ?

[iramiller]

We ended creating a "git-filter fork" of the cosmovisor code into our own repo to fix this issue. This seemed like the best way forward as we also wanted to see additional changes such as taking a snapshot of the data directory before performing an upgrade (per best practice) in case a rollback is required as well as some useful docker packaging.

https://github.com/provenance-io/cosmovisor

Let me know if there is interest in seeing any of that work come back into the SDK.

[clevinson]

Hey @iramiller -- we'll be merging #8590 into a v1.0 of cosmovisor soon (in conjunction with v0.43 release of the SDK). Hopefully that resolves this issue. Will you be able to see if the changes in #8590 take care of this?

[clevinson]

The other pieces you mention look like they could be great to have in cosmovisor. Maybe let's see what the v1.0 release looks like with the change to file based handlers, and then if there's interest in opening a PR upstream with the remainder of pieces from your fork that would still be useful we can get some folks on our team to review it.

[iramiller]

Well @clevinson -- #9384 directly implements the remediation as outlined above in this issue (even though these two are not linked at the moment). The other changes in #8590 are nice too as relying on logs at all seems like a poor design.

[robert-zaremba]

This is solved in: #8590 (still in review and testing).