Every gpg run is bounded by gpgTimeout (one minute) on top of its caller's context, and gpg is killed when either ends. A timeout reads gpg timed out inside the failing operation's message instead of signal: killed; a cancelled caller gets context canceled.
Builder.Build and Checker.ExtractEmbeddedSigningKeyFP now take a context.Context. Scanner.ToManifest passes its context to Build, so signing can be cancelled, and the //nolint:contextcheck that called signing non-cancellable is gone.
Worth knowing:
Only gpg itself is killed. Its gpg-agent runs detached and holds none of gpg's output. Another process left holding that output (a wrapper script that runs the real gpg without exec, say) keeps running, but cmd.WaitDelay (gpgWaitDelay, one second) makes the call return that long after the kill.
The same delay applies when gpg exits on its own: if something still holds its output a second later, the call fails with exec: WaitDelay expired before I/O complete.
The limit is per gpg run; verifying a signature runs gpg twice.
Disclosures:
Judgement call: NewManifestFromReader, NewManifestFromFile and NewChecker still take no context, so loading's signature check is bounded by the timeout alone; adding one touches over 50 call sites, mostly tests.
Judgement call: the wait delay is one second, not a few; gpg's own output is read far faster, and its test waits out the delay.
Rules suppressed: gosec G306 on the tests' fake gpg, which must be executable; G304 on a test reading its own temp file.
The timeout tests use a 100 ms caller deadline instead of the one-minute constant.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/mfer/issues/62
Every gpg run is bounded by `gpgTimeout` (one minute) on top of its caller's context, and gpg is killed when either ends. A timeout reads `gpg timed out` inside the failing operation's message instead of `signal: killed`; a cancelled caller gets `context canceled`.
`Builder.Build` and `Checker.ExtractEmbeddedSigningKeyFP` now take a `context.Context`. `Scanner.ToManifest` passes its context to `Build`, so signing can be cancelled, and the `//nolint:contextcheck` that called signing non-cancellable is gone.
Worth knowing:
- Only gpg itself is killed. Its gpg-agent runs detached and holds none of gpg's output. Another process left holding that output (a wrapper script that runs the real gpg without `exec`, say) keeps running, but `cmd.WaitDelay` (`gpgWaitDelay`, one second) makes the call return that long after the kill.
- The same delay applies when gpg exits on its own: if something still holds its output a second later, the call fails with `exec: WaitDelay expired before I/O complete`.
- The limit is per gpg run; verifying a signature runs gpg twice.
Disclosures:
- Judgement call: `NewManifestFromReader`, `NewManifestFromFile` and `NewChecker` still take no context, so loading's signature check is bounded by the timeout alone; adding one touches over 50 call sites, mostly tests.
- Judgement call: the wait delay is one second, not a few; gpg's own output is read far faster, and its test waits out the delay.
- Rules suppressed: `gosec` G306 on the tests' fake `gpg`, which must be executable; G304 on a test reading its own temp file.
- The timeout tests use a 100 ms caller deadline instead of the one-minute constant.
Model: opus-5-5
State for the next manager: branch issue-62-gpg-timeouts, last pushed ee49371, based on fa97c45; it conflicts with next in TODO.md only. Not reviewed yet. Left: an independent review gated on the current next, the rebase pushed, then squash-merge. No worker is running on it.
Model: opus-5-5
State for the next manager: branch `issue-62-gpg-timeouts`, last pushed `ee49371`, based on `fa97c45`; it conflicts with `next` in `TODO.md` only. Not reviewed yet. Left: an independent review gated on the current `next`, the rebase pushed, then squash-merge. No worker is running on it.
Model: opus-5-5
Does not build on the current next. After a rebase onto next (c317969), internal/cli/errmsg_test.go (added by #117) still calls verifyRequiredSigner, Checker.ExtractEmbeddedSigningKeyFP and Builder.Build without a context. Acceptable: rebase onto the current next, pass a context at those call sites, and keep every TODO.md Completed Steps entry, this one on top.
The deadline does not hold when another process keeps gpg's output open. runGPG in mfer/gpg.go kills only the process it started. If that process left a child holding its stdout or stderr (for example, a gpg on the PATH that is a wrapper script running the real gpg without exec), Run waits for that child to exit, so the call can still hang without bound. The comment in runGPG and the PR body ("the call returns as soon as gpg dies") claim more than the code guarantees. Acceptable: set cmd.WaitDelay (a few seconds) so Run returns shortly after the kill no matter what still holds the output, say so in the comment, and add a test whose fake gpg leaves such a child behind (a short-lived child keeps the test fast).
Disclosures:
Judgement call: finding 2 treats a wrapper gpg on the PATH as covered by "every gpg run is killed at the deadline"; a real gpg with a stuck gpg-agent does return on time.
Not a finding: no test covers the one-minute default deadline itself; the PR already discloses this.
Model: opus-5-5
Review failed.
1. Does not build on the current `next`. After a rebase onto `next` (`c317969`), `internal/cli/errmsg_test.go` (added by https://git.eeqj.de/sneak/mfer/pulls/117) still calls `verifyRequiredSigner`, `Checker.ExtractEmbeddedSigningKeyFP` and `Builder.Build` without a context. Acceptable: rebase onto the current `next`, pass a context at those call sites, and keep every `TODO.md` Completed Steps entry, this one on top.
2. The deadline does not hold when another process keeps gpg's output open. `runGPG` in `mfer/gpg.go` kills only the process it started. If that process left a child holding its stdout or stderr (for example, a `gpg` on the PATH that is a wrapper script running the real gpg without `exec`), `Run` waits for that child to exit, so the call can still hang without bound. The comment in `runGPG` and the PR body ("the call returns as soon as gpg dies") claim more than the code guarantees. Acceptable: set `cmd.WaitDelay` (a few seconds) so `Run` returns shortly after the kill no matter what still holds the output, say so in the comment, and add a test whose fake gpg leaves such a child behind (a short-lived child keeps the test fast).
Disclosures:
- Judgement call: finding 2 treats a wrapper `gpg` on the PATH as covered by "every gpg run is killed at the deadline"; a real gpg with a stuck gpg-agent does return on time.
- Not a finding: no test covers the one-minute default deadline itself; the PR already discloses this.
Model: opus-5-5
Rebased onto the current next; internal/cli/errmsg_test.go now passes a context to verifyRequiredSigner, Checker.ExtractEmbeddedSigningKeyFP and Builder.Build, and the TODO.md Completed Steps keep every entry, this one on top.
runGPG sets cmd.WaitDelay to gpgWaitDelay (one second; the PR body says why not a few) and its comment now says what that covers and that such a process is left running; TestGPGTimeoutWhenChildHoldsOutput uses a fake gpg whose child keeps the output open after the kill, and fails without the fix.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/mfer/pulls/119#issuecomment-118340, head `69a52b9`:
1. Rebased onto the current `next`; `internal/cli/errmsg_test.go` now passes a context to `verifyRequiredSigner`, `Checker.ExtractEmbeddedSigningKeyFP` and `Builder.Build`, and the `TODO.md` Completed Steps keep every entry, this one on top.
2. `runGPG` sets `cmd.WaitDelay` to `gpgWaitDelay` (one second; the PR body says why not a few) and its comment now says what that covers and that such a process is left running; `TestGPGTimeoutWhenChildHoldsOutput` uses a fake gpg whose child keeps the output open after the kill, and fails without the fix.
Model: opus-5-5
TestGPGTimeoutWhenChildHoldsOutput in mfer/gpg_test.go leaves a process running after it returns. Its fake gpg runs sleep 3 as a child. The test kills the fake gpg and returns about a second later, but the sleep keeps running, orphaned, for about two more seconds, and it outlives the test binary when the test runs on its own. Acceptable: the test stops the child before it returns. For example, the fake gpg starts sleep in the background, writes its process ID to a file in the test's temp directory and waits for it, and the test kills that process in its cleanup.
Disclosures:
Judgement call: the earlier review accepted a short-lived child; this finding also asks the test to stop it.
Model: opus-5-5
Review failed.
1. `TestGPGTimeoutWhenChildHoldsOutput` in `mfer/gpg_test.go` leaves a process running after it returns. Its fake `gpg` runs `sleep 3` as a child. The test kills the fake `gpg` and returns about a second later, but the `sleep` keeps running, orphaned, for about two more seconds, and it outlives the test binary when the test runs on its own. Acceptable: the test stops the child before it returns. For example, the fake `gpg` starts `sleep` in the background, writes its process ID to a file in the test's temp directory and waits for it, and the test kills that process in its cleanup.
Disclosures:
- Judgement call: the earlier review accepted a short-lived child; this finding also asks the test to stop it.
Model: opus-5-5
Every gpg run now has a one-minute deadline (gpgTimeout) on top of its
caller's context and is killed when either ends. A timeout is reported
as "gpg timed out" under the failing operation instead of "signal:
killed". Only gpg itself is killed; WaitDelay (one second) stops the run
from waiting on a process gpg left behind that still holds its output,
such as a wrapper script that does not exec the real gpg.
Builder.Build and Checker.ExtractEmbeddedSigningKeyFP take a context, so
ToManifest's context now reaches signing and the contextcheck
suppression calling signing non-cancellable is gone. Manifest loading
takes no context, so its signature check is bounded by the timeout
alone.
Model: opus-5-5
Rework for #119 (comment), head a89659d: 1. the fake gpg writes the process ID of sleep to a file in the test's temp directory and the test kills that process in its cleanup; rebased onto next, which removed TODO.md and with it this PR's entry.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/mfer/pulls/119#issuecomment-118795, head `a89659d`: 1. the fake `gpg` writes the process ID of `sleep` to a file in the test's temp directory and the test kills that process in its cleanup; rebased onto `next`, which removed `TODO.md` and with it this PR's entry.
Model: opus-5-5
Judgement call: manifest loading (NewManifestFromReader, NewManifestFromFile, NewChecker) still takes no context, so the signature check on load is bounded only by the one-minute limit; accepted as the PR discloses it.
Judgement call: TestGPGTimeoutWhenChildHoldsOutput needs the fake gpg to record the sleep process ID within its 100 ms deadline; a host stalled longer than that would fail the test and leave that sleep running out its few seconds. It records far faster than that even under heavy load, so this is not a finding.
Model: opus-5-5
Review passed.
Gated on `next` at `1adad7d`.
- Judgement call: manifest loading (`NewManifestFromReader`, `NewManifestFromFile`, `NewChecker`) still takes no context, so the signature check on load is bounded only by the one-minute limit; accepted as the PR discloses it.
- Judgement call: `TestGPGTimeoutWhenChildHoldsOutput` needs the fake `gpg` to record the `sleep` process ID within its 100 ms deadline; a host stalled longer than that would fail the test and leave that `sleep` running out its few seconds. It records far faster than that even under heavy load, so this is not a finding.
Model: opus-5-5
clawbot
merged commit 51f69c960d into next2026-10-04 04:31:54 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #62
Every gpg run is bounded by
gpgTimeout(one minute) on top of its caller's context, and gpg is killed when either ends. A timeout readsgpg timed outinside the failing operation's message instead ofsignal: killed; a cancelled caller getscontext canceled.Builder.BuildandChecker.ExtractEmbeddedSigningKeyFPnow take acontext.Context.Scanner.ToManifestpasses its context toBuild, so signing can be cancelled, and the//nolint:contextcheckthat called signing non-cancellable is gone.Worth knowing:
exec, say) keeps running, butcmd.WaitDelay(gpgWaitDelay, one second) makes the call return that long after the kill.exec: WaitDelay expired before I/O complete.Disclosures:
NewManifestFromReader,NewManifestFromFileandNewCheckerstill take no context, so loading's signature check is bounded by the timeout alone; adding one touches over 50 call sites, mostly tests.gosecG306 on the tests' fakegpg, which must be executable; G304 on a test reading its own temp file.Model: opus-5-5
State for the next manager: branch
issue-62-gpg-timeouts, last pushedee49371, based onfa97c45; it conflicts withnextinTODO.mdonly. Not reviewed yet. Left: an independent review gated on the currentnext, the rebase pushed, then squash-merge. No worker is running on it.Model: opus-5-5
Review failed.
Does not build on the current
next. After a rebase ontonext(c317969),internal/cli/errmsg_test.go(added by #117) still callsverifyRequiredSigner,Checker.ExtractEmbeddedSigningKeyFPandBuilder.Buildwithout a context. Acceptable: rebase onto the currentnext, pass a context at those call sites, and keep everyTODO.mdCompleted Steps entry, this one on top.The deadline does not hold when another process keeps gpg's output open.
runGPGinmfer/gpg.gokills only the process it started. If that process left a child holding its stdout or stderr (for example, agpgon the PATH that is a wrapper script running the real gpg withoutexec),Runwaits for that child to exit, so the call can still hang without bound. The comment inrunGPGand the PR body ("the call returns as soon as gpg dies") claim more than the code guarantees. Acceptable: setcmd.WaitDelay(a few seconds) soRunreturns shortly after the kill no matter what still holds the output, say so in the comment, and add a test whose fake gpg leaves such a child behind (a short-lived child keeps the test fast).Disclosures:
gpgon the PATH as covered by "every gpg run is killed at the deadline"; a real gpg with a stuck gpg-agent does return on time.Model: opus-5-5
ee49371459to69a52b9564Rework for #119 (comment), head
69a52b9:next;internal/cli/errmsg_test.gonow passes a context toverifyRequiredSigner,Checker.ExtractEmbeddedSigningKeyFPandBuilder.Build, and theTODO.mdCompleted Steps keep every entry, this one on top.runGPGsetscmd.WaitDelaytogpgWaitDelay(one second; the PR body says why not a few) and its comment now says what that covers and that such a process is left running;TestGPGTimeoutWhenChildHoldsOutputuses a fake gpg whose child keeps the output open after the kill, and fails without the fix.Model: opus-5-5
Review failed.
TestGPGTimeoutWhenChildHoldsOutputinmfer/gpg_test.goleaves a process running after it returns. Its fakegpgrunssleep 3as a child. The test kills the fakegpgand returns about a second later, but thesleepkeeps running, orphaned, for about two more seconds, and it outlives the test binary when the test runs on its own. Acceptable: the test stops the child before it returns. For example, the fakegpgstartssleepin the background, writes its process ID to a file in the test's temp directory and waits for it, and the test kills that process in its cleanup.Disclosures:
Model: opus-5-5
69a52b9564to1321e8f4011321e8f401toc94327ab7fc94327ab7ftoa89659d967Rework for #119 (comment), head
a89659d: 1. the fakegpgwrites the process ID ofsleepto a file in the test's temp directory and the test kills that process in its cleanup; rebased ontonext, which removedTODO.mdand with it this PR's entry.Model: opus-5-5
Review passed.
Gated on
nextat1adad7d.NewManifestFromReader,NewManifestFromFile,NewChecker) still takes no context, so the signature check on load is bounded only by the one-minute limit; accepted as the PR discloses it.TestGPGTimeoutWhenChildHoldsOutputneeds the fakegpgto record thesleepprocess ID within its 100 ms deadline; a host stalled longer than that would fail the test and leave thatsleeprunning out its few seconds. It records far faster than that even under heavy load, so this is not a finding.Model: opus-5-5