Reenable CI fuzzing of pull requests

[EliahKagan]

Per EliahKagan#1 (comment), it looks like I can't investigate this properly in a fork-internal PR. Ideally this will turn into a mergeable PR, but that is not the case yet, and it is not guaranteed.

---

The error is:

error[E0658]: `#[diagnostic]` attribute name space is experimental
   --> /rust/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.210/src/de/mod.rs:536:5
    |
536 |     diagnostic::on_unimplemented(
    |     ^^^^^^^^^^
    |
    = note: see issue #111996 <https://github.com/rust-lang/rust/issues/111996> for more information
    = help: add `#![feature(diagnostic_namespace)]` to the crate attributes to enable
    = note: this compiler was built on 2024-02-11; consider upgrading it if it is out of date

As noted in EliahKagan#1 (comment), this might be possible to fix by downgrading a dependency, though I would be reluctant to modify versions in Cargo.lock directly, which I expect could be undone automatically when changes are made to any Cargo.toml file anywhere in the workspace.

What I am not sure of is whether it should be done that way, or by using a newer toolchain by updating the Docker image. rust-lang/rust#111996 is closed as completed. Does that mean using a newer rustc would avoid the problem?

[EliahKagan]

@Byron In EliahKagan#1 (comment) you had suggested that downgrading serde in Cargo.lock might help. This makes sense, since their versions were upgraded in #1536, where the breakage seems to have started. But does the build in the CI fuzzing job actually use locked dependencies? Is it intended to?

• When I first experimented here, a few minutes ago, with adjusting the version, I had failed to change the associated hashes, and the fuzzing build still got further than I would expect. See, for example, 67024c5.
• More decisively, at aafa8b1, where I tried to set the version to 1.0.204, it is nonetheless using 1.0.210, which is currently the latest version. This seems to indicate that Cargo.lock has no effect here.

[Byron]

Thanks for looking into this! Admittedly I completely forget about it already and CI-Fuzzing would be lost in time.

@Byron In EliahKagan#1 (comment) you had suggested that downgrading serde in Cargo.lock might help. This makes sense, since their versions were upgraded in #1536, where the breakage seems to have started. But does the build in the CI fuzzing job actually use locked dependencies? Is it intended to?

It's true, I also have the impression it uses the the latest possible versions of dependencies, so Cargo.lock shouldn't matter for it.

So updating the image to use a more recent Rust version certainly would help. On top of that, I think the minimal rust-version supported by serde should also be adjusted to capture this dependency.

[EliahKagan]

So updating the image to use a more recent Rust version certainly would help.

I've opened google/oss-fuzz#12512 for this.

I assume fuzzing is completely broken and not just when triggered by our CI, but I have only actually verified that it is broken on CI. I'm not sure how to check other runs, or even if I am able to do so, since I have no special access. (I have access to oss-fuzz reports for GitPython but not, as far as I know, for gitoxide.)

If that change is accepted and if it fixes the problem, this PR could then be merged to reenable CI fuzzing of pull requests, but if that is to be done, then the second commit here should either be dropped via force push, or reverted. This is because downgrading serde-related crates in Cargo.lock is ineffective at fixing fuzzing, and probably undesirable otherwise.

To aid with that, I've gone ahead and reverted that commit. But we can still force push back to the first commit, of course.

On top of that, I think the minimal rust-version supported by serde should also be adjusted to capture this dependency.

Based on serde-rs/serde#2770, and specifically serde-rs/serde#2770 (comment), it looks like the issue is known and only occurs when using old nightly compilers, and that serde only attempts to support current nightly compilers. So based on that my guess is that there may not be anything to do in serde for this.

Eventually, I think the default version oss-fuzz uses of the nightly Rust compiler will be upgraded. See google/oss-fuzz#12410. But we shouldn't have to wait for that to happen to fix this. As noted there and in google/oss-fuzz#12512, the latest nightly is being used in a couple of projects to work around this (google/oss-fuzz#12404, google/oss-fuzz#12409).

[Byron]

Thanks a lot for the summary, I am looking forward to upstream merge so CI will be back here.

[EliahKagan]

This is working now, since google/oss-fuzz#12512.

---

If this pull request is merged, then it might be a good idea, afterwards, to add the CIFuzz job as a required check for automatic merging. Considerations:

• If it is added, then it could fail due to breakages related to using a nightly toolchain, which might not always be relevant to the content of the pull request being fuzzed. (This could happen if the Docker image is rebuilt, but it is more likely to happen the same way as it happened before: a newer version of a dependency does not support a previous nightly toolchain.)
• But if it is not added, then pull requests that fail fuzzing may be automatically merged before fuzzing completes. Since CI fuzzing is not being done on the push trigger, this would then wait until oss-fuzz fuzzing is performed. So to actually get the intended benefit of fuzzing pull requests, the CIFuzz job should be me made "required" so it blocks automatic merging.
• The CIFuzz job does appear to be the longest-running job, though. So making it required may increase the amount of time it takes for PRs to be merged automatically.

Breakages (if they do not reflect a possible bug due to changes a PR proposes) can reasonably be addressed by temporarily disabling CI fuzzing, as was done in #1536. (In unusual cases, they could possibly be addressed by manually merging pull requests for which fuzzing has failed but other checks have passed.)

This pull request does not attempt to introduce changes to cause fuzzing to be effectively required, because I don't know of a way to do that by editing workflows that is clearly better than configuring it manually.

#1536, which (among other changes) removed the CIFuzz job that this PR restores, was merged on August 23, which was before #1551 was opened and merged on August 25. Reviewing #1551 therefore did not involve any considerations of whether fuzzing failures should block PRs. Because the CIFuzz job is in a different GitHub Actions workflow file from the jobs affected by #1551, a cross-workflow dependency would be needed to include it in a similar way. Since this is not necessarily clearer or more elegant than configuring it as required, I have not made that change here.

[Byron]

Thanks a lot, that's great news!

Then it seems that after merging, this job will be made a required one to allow it to block merges, accepting the Risk that eventually, it will fail for other reasons.

Could you tune the fuzz-seconds fields to bring the job down to the longest running non-fuzz job? The suggestion should be fine, but might need more tuning.

[Byron]

Suggested change

	fuzz-seconds: 600
	fuzz-seconds: 300

This should bring the Job duration down to ~15 minutes, making it run about as long as the Windows Job, which seems slowest now.

[EliahKagan]

The oss-fuzz documentation recommends against decreasing fuzz-seconds below 600:

5. Set the value of fuzz-seconds. The longest time that the project maintainers are acceptable with should be used. This value should be at minimum 600 seconds and scale with project size.

I don't know that this necessarily means we shouldn't use a smaller value, but I would want to understand why 600 seconds is the recommended minimum before doing so. I am also unclear on whether smaller values are disregarded and treated the same as 600 seconds, or honored but not recommended, possibly due to diminishing value.

There doesn't seem to be a specific explanation for this in the documentation or elsewhere. This was added along with various other material to the documentation in google/oss-fuzz#3572. I can inquire at https://github.com/google/oss-fuzz/discussions about the reason, if decreasing it is something you think is worth pursuing and you want my specific input on it. This might be a way to figure out how best to tune it.

[Byron]

Thanks for looking into this!

Then I think I will… instead not enforce CI-Fuzzing to finish before merging.
If it fails later, I will still get an email.

When working on a parser it's possible to just wait for it to finish just to be sure there is no obvious issue that a fuzzer would catch within a couple of missing.