Raising this as an issue first per CONTRIBUTING.md, and not opening a PR until you say it is worth having.
CONTRIBUTING.md has a ## Close Policy section, and it covers the grounds on which a PR may be closed: no approved issue, not run locally, mixed purposes, inactivity. It says nothing about what happens to the findings inside a thread once it closes, and that is where something can quietly go missing.
The case that prompted this is one of mine, and the close was correct. #3795 proposed bounding the runtime's source forwarding channel. af9ce9548 (#3855) made its premise obsolete, since one batch in flight means the channel cannot grow, so you called for closing it and I did. What neither of us said at the time was that the premise only holds while the runtime answers inside the SDK's 30s BATCH_RESULT_TIMEOUT. Past that the SDK stops waiting, NACKs and polls again, so the channel grows about one batch per timeout, and MAX_CONSECUTIVE_NACKS does not bound it because any Ack resets the counter.
That surviving half lived only in a closed PR's comment thread for several days. It is now #3981, but only because I asked whether closing had dropped anything. Nothing about the process would have surfaced it.
What I would like to add is short, and additive rather than a rewrite of what is there:
- Before closing, name the findings inside the thread and say where each one goes. A finding that outlives the change it arrived in needs its own issue.
- Leave a comment on the closed thread pointing at whatever it became. That thread is where someone asking "why was this closed, and did anything fall through" looks first, and it is the step easiest to skip.
Both halves already exist for the case above, if a worked example helps:
The second is the part I would not have done unprompted, and it is the one that caught a documentation claim that had gone wrong for the same reason the finding went missing.
It applies to whoever closes, maintainers included, and I think it saves you time rather than costing it: the alternative is the finding resurfacing months later as a bug report with no history attached.
Happy to write it if you want it. Equally happy to be told the policy section is deliberately about grounds only and this belongs somewhere else, or nowhere.
Raising this as an issue first per
CONTRIBUTING.md, and not opening a PR until you say it is worth having.CONTRIBUTING.mdhas a## Close Policysection, and it covers the grounds on which a PR may be closed: no approved issue, not run locally, mixed purposes, inactivity. It says nothing about what happens to the findings inside a thread once it closes, and that is where something can quietly go missing.The case that prompted this is one of mine, and the close was correct. #3795 proposed bounding the runtime's source forwarding channel.
af9ce9548(#3855) made its premise obsolete, since one batch in flight means the channel cannot grow, so you called for closing it and I did. What neither of us said at the time was that the premise only holds while the runtime answers inside the SDK's 30sBATCH_RESULT_TIMEOUT. Past that the SDK stops waiting, NACKs and polls again, so the channel grows about one batch per timeout, andMAX_CONSECUTIVE_NACKSdoes not bound it because anyAckresets the counter.That surviving half lived only in a closed PR's comment thread for several days. It is now #3981, but only because I asked whether closing had dropped anything. Nothing about the process would have surfaced it.
What I would like to add is short, and additive rather than a rewrite of what is there:
Both halves already exist for the case above, if a worked example helps:
The second is the part I would not have done unprompted, and it is the one that caught a documentation claim that had gone wrong for the same reason the finding went missing.
It applies to whoever closes, maintainers included, and I think it saves you time rather than costing it: the alternative is the finding resurfacing months later as a bug report with no history attached.
Happy to write it if you want it. Equally happy to be told the policy section is deliberately about grounds only and this belongs somewhere else, or nowhere.