Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blockstore: OpenReadWrite should not modify if it refuses to resume #248

Merged
merged 1 commit into from
Oct 1, 2021
Merged

blockstore: OpenReadWrite should not modify if it refuses to resume #248

merged 1 commit into from
Oct 1, 2021

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Sep 30, 2021

(see commit message)

If OpenReadWrite sees a good reason to refuse to resume,
such as the provided roots not matching what's on disk,
it should not truncate or un-finalize the file on disk.

Add a test that exercises that edge case,
and also verifies that the file isn't modified.

To fix the problem, carefully move truncation and un-finalization
past the point where we've checked the existing header.

Note that we may still leave a broken file if we encounter an error
later on, such as when writing to the file or building the index.
If we want those to not break the input file we'll need bigger changes,
such as writing to a temporary file and atomically replacing the final
file at the very end.

For now, at least we can avoid a common breakage case.

Before the fix, the test would fail:

    Error Trace: readwrite_test.go:621
    Error:       Not equal:
                   expected: 521708
                   actual  : 479958
    Test:        TestReadWriteResumptionMismatchingRootsIsError

Fixes #247.
@mvdan
Copy link
Contributor Author

mvdan commented Sep 30, 2021

I ended up splitting the "temproary file" idea mentioned in the commit message into #249.

v2/blockstore/readwrite_test.go Show resolved Hide resolved
@mvdan mvdan merged commit 4e0a1fa into ipld:master Oct 1, 2021
@mvdan mvdan deleted the readwrite-error-modify branch October 4, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants