inconsistent state when concern raised after FCP completion

[tlyu]

Summary

It looks like if a reviewer raises a concern after FCP completion, a tracking issue can get into an inconsistent state. This allows the FCP to restart after the reviewer resolves the concern, but rustbot will not notice when this new FCP has ended.

There's probably a genuine policy choice to make here; either:

• After FCP completion, new concerns won't change the rfcbot state at all.
• After FCP completion, new concerns fully return the proposal to a proposed-final-comment-period state, including removing finished-final-comment-period (and maybe also removing to-announce), and corresponding internal rfcbot state changes. This would allow automatic restart and completion of a new FCP.

The current behavior is internally inconsistent and probably not as helpful. Maybe rfcbot should give the team a chance to decide whether a new concern can return a tracking issue to a proposed FCP state, but I imagine that might add unwanted complexity.

Details

In rust-lang/rust#87096, a concern was raised after the FCP completed. rustbot added the proposed-final-comment-period label, but did not remove the finished-final-comment-period label. I assume it also did not change fcp_closed to false, but that might be a piece of database state that's not visible to me? rfcbot initiated a new FCP after the concern resolved, but did not notice when the FCP had ended.

It looks like rfcbot checks the database state, not the state of the issue labels, to determine the set of finished FCPs to close: src/github/nag.rs
I couldn't find any code that resyncs the database state to the labels in a relevant way.

Possible solutions

Probably the easiest thing to do is to unconditionally embed one of the above policy choices into rfcbot. Probably it's easier to make the policy choice to allow a reviewer to raise a concern after FCP completion, under the assumption that a reviewer wouldn't do so without knowing the FCP had already completed. (How valid is this assumption?)

Another possibility is that a team member could manually remove the finished-final-comment-period (and/or to-announce) label to signal the policy choice to allow FCP restart. This might require rfcbot to rescan all comments that come after the comment announcing the most recent FCP completion, so might be undesired complexity.

Alternatively, maybe add a command (or a modifier to the concern command?) to explicitly allow the issue to return to a proposed FCP state. Concerns raised after FCP completion would then not change rfcbot state unless explicitly allowed, and rfcbot might make a warning comment that the concern will be ignored unless the prior FCP completion is explicitly overridden.

[joshtriplett]

I think the second of those two policies makes more sense: upon filing a concern go to proposed-final-comment-period (removing any other FCP-related labels), and upon resolving the concern, go to the appropriate FCP state based on the amount of time since having proposed FCP.

I don't think this needs to be an option.

[tlyu]

I think the second of those two policies makes more sense: upon filing a concern go to proposed-final-comment-period (removing any other FCP-related labels), and upon resolving the concern, go to the appropriate FCP state based on the amount of time since having entered FCP.

I don't think this needs to be an option.

Are you suggesting that an additional FCP is unnecessary if a concern is raised and resolved after the FCP completes? Note that the current code appears to reset the FCP clock if a concern is raised during FCP. I was suggesting that for consistency, a new FCP begins after the concern (raised after FCP completion) is resolved.

[joshtriplett]

The goal of the FCP period is to make sure that everyone has a chance to respond, even if something is posted, proposed for merge, and most people check their boxes right away. However, the goal isn't for everyone to raise concerns sequentially, and have ten days since the last one. Ten days since proposing FCP suffices to make sure people see the issue before we take action; certainly ten days since having enough checkboxes suffices.