mirror: fix non-determinism in case two packages have the same path #143169
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
rickystewart
force-pushed
the
mirror-bug
branch
from
57fd6aa to
dce1a93
Compare
rickystewart
added
backport-24.1.x
This code previously assumed that `go mod download -json` would not produce two different versions of the same dependency with the same path. This is typically a sensible assumption but no longer holds in some niche scenarios. We use [replace](https://github.com/cockroachdb/cockroach/blob/65b2ed4fbdf5502f3fbe0af4ddbd30a7ac7eabb4/go.mod#L500) in `go.mod` to effectively import two different versions of the same dependency with the same path. This results in non-determinism in the mirroring code with respect to which version of the dependency we select. We now disambiguate with a path/version pair, which will be unique. We also add some additional validation to check assumptions so if these assumptions are ever broken in the future, the tool will fail loudly instead of proceeding silently and performing a potentially harmful operation. Fixes cockroachdb#143168 Epic: CRDB-17171 Release note: None
rickystewart
force-pushed
the
mirror-bug
branch
from
dce1a93 to
87769f7
Compare
| // not. We can import B many times under different | ||
| // imported names. | ||
| if _, ok := ret[mod.Path]; ok { | ||
| panic(fmt.Sprintf("found duplicate imported path: %s. This is a bug. Go tell dev-inf about it.", mod.Path)) |
|
bors r=rail |
143100: kv: use origin timestamp to validate lww semantics r=jeffswenson a=jeffswenson The PR updates the implementation of put, cput, delete, and range delete to return an error if a replicated write with an origin timestamp would overwrite a value with a more recent origin or MVCC timestamp. No explicit version checks are included. In a mixed-version environment, a SQL writer running 25.1 and writing to a 25.2 KV server will encounter "condition failed" errors if an insert would overwrite a tombstone and retry the write until it is upgraded or the LDR PTS ages out. This is acceptable, as LDR remains in preview and the SQL writer is neither the default nor the recommended writer. Release note: none Epic: [CRDB-48647](https://cockroachlabs.atlassian.net/browse/CRDB-48647) 143169: mirror: fix non-determinism in case two packages have the same path r=rail a=rickystewart This code previously assumed that `go mod download -json` would not produce two different versions of the same dependency with the same path. This is typically a sensible assumption but no longer holds in some niche scenarios. We use [replace](https://github.com/cockroachdb/cockroach/blob/65b2ed4fbdf5502f3fbe0af4ddbd30a7ac7eabb4/go.mod#L500) in `go.mod` to effectively import two different versions of the same dependency with the same path. This results in non-determinism in the mirroring code with respect to which version of the dependency we select. We now disambiguate with a path/version pair, which will be unique. We also add some additional validation to check assumptions so if these assumptions are ever broken in the future, the tool will fail loudly instead of proceeding silently and performing a potentially harmful operation. Fixes #143168 Epic: CRDB-17171 Release note: None Co-authored-by: Jeff Swenson <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
|
Build failed (retrying...): |
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #143168: branch-release-24.1, branch-release-24.3, branch-release-25.1. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 87769f7 to release-24.1: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.1.x failed. See errors above. error creating merge commit from 87769f7 to release-24.3: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.3.x failed. See errors above. error creating merge commit from 87769f7 to release-25.1: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 25.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This code previously assumed that
go mod download -jsonwould not produce two different versions of the same dependency with the same path. This is typically a sensible assumption but no longer holds in some niche scenarios. We use replace ingo.modto effectively import two different versions of the same dependency with the same path. This results in non-determinism in the mirroring code with respect to which version of the dependency we select.We now disambiguate with a path/version pair, which will be unique. We also add some additional validation to check assumptions so if these assumptions are ever broken in the future, the tool will fail loudly instead of proceeding silently and performing a potentially harmful operation.
Fixes #143168
Epic: CRDB-17171
Release note: None