Re-mint watchpr's Gitea token when it expires #10

2026-09-09T21:16:02+10:00

unkin-agent commented

2026-09-09 21:16:02 +10:00

Vault-minted Gitea tokens last ~1h, far less than a watch, so every poll past expiry 401'd into a warning while watchpr looked healthy and saw nothing.

retry a rejected request once with a freshly minted token
abort the watch (non-zero exit) when the fresh token is rejected too
keep 5xx/network errors on their existing retry path
poll anonymously when no token can be minted; only a real 401/403 mints

Vault-minted Gitea tokens last ~1h, far less than a watch, so every poll past expiry 401'd into a warning while watchpr looked healthy and saw nothing. - retry a rejected request once with a freshly minted token - abort the watch (non-zero exit) when the fresh token is rejected too - keep 5xx/network errors on their existing retry path - poll anonymously when no token can be minted; only a real 401/403 mints

unkin-agent added 1 commit 2026-09-09 21:16:02 +10:00

Re-mint watchpr's Gitea token when it expires

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

7ef0e28e96

Vault-minted Gitea tokens last ~1h, far less than a watch, and every poll
past expiry 401'd into a warning while watchpr looked healthy.

- retry a rejected request once with a freshly minted token
- abort the watch when the fresh token is rejected too
- poll anonymously when no token can be minted, mint only on a real 401/403

unkin-agent commented

2026-09-09 21:32:40 +10:00

Reviewed diff + go test -race ./... (all pass). Core correctness checks out:

do/attempt split (internal/agent/gitea.go:394-436): body is marshalled once to []byte in do, a fresh bytes.NewReader is built per attempt call, so the replay is a genuine second request, not a re-read of a drained reader. Confirmed by TestRemintReplaysRequestBody.
Retry happens at most once, and a failed re-mint can't loop (gitea.go:407-416): do calls attempt a maximum of twice; a Refresh() error returns immediately wrapped as an auth error, never retried again. TestAuthFailureSurvivesRemint/TestRemintErrorIsReportedAsAuthFailure back this up.
Abort semantics (watch.go:110-113, cmd/watchpr/main.go:134-138): a 401/403 that survives re-mint aborts Watch non-zero; 5xx/network errors keep the old warn-and-continue path (TestServerErrorDoesNotRemint, TestWatchContinuesPastServerError).
Anonymous mode: attempt omits Authorization entirely when Token=="" (gitea.go:430-433); do only calls Refresh when IsAuthError(err), so a 200 never mints (TestAnonymousPollingNeverMints). Startup failure to mint is now a warning, not fatal, only in watchpr's clientFor — agentpr's client() (cmd/agentpr/main.go:47-53) is untouched and still fails fast, and RefreshGiteaToken re-mints from the same env-derived role/creds path, so no command can end up acting as a different identity than before.

Two non-blocking design notes, since you asked me to reason about thundering-herd behaviour explicitly:

RefreshGiteaToken (internal/agent/token.go:109-115) has no single-flight/coalescing: it unconditionally re-mints on every call. The global mutex serializes concurrent callers (no data race, no lost update) but does not dedupe them — N concurrent 401s on a shared client would still cost N sequential Vault mints, not 1. Not reachable today: Watch drives one *GiteaClient from a single goroutine, and every cmd/* entrypoint does the same, so this is latent rather than a live bug. Worth a comment or a follow-up if a client is ever shared across goroutines.
Relatedly, GiteaClient.Token is mutated in do() (gitea.go:415) with no synchronization on the client itself — fine under the current one-goroutine-per-client usage, would race under concurrent use of the same client instance.

Minor scope note: the PR bundles two related-but-distinct concerns — (1) mid-watch token expiry (remint/replay/abort) and (2) anonymous polling when no token can be minted at all (a startup-time fallback, previously fatal). Both are small and well tested, so I'm not requesting a split, just flagging it since (2) trades a fast-fail on Vault misconfiguration for silent-if-nobody-reads-stderr degraded operation on public repos. That tradeoff is intentional and documented (AGENTS.md, PR body), just worth being deliberate about.

PR body is within the size/format convention (6 lines, 412 chars, why + present-tense bullets).

Reviewed diff + `go test -race ./...` (all pass). Core correctness checks out: - `do`/`attempt` split (internal/agent/gitea.go:394-436): body is marshalled once to `[]byte` in `do`, a fresh `bytes.NewReader` is built per `attempt` call, so the replay is a genuine second request, not a re-read of a drained reader. Confirmed by `TestRemintReplaysRequestBody`. - Retry happens at most once, and a failed re-mint can't loop (gitea.go:407-416): `do` calls `attempt` a maximum of twice; a `Refresh()` error returns immediately wrapped as an auth error, never retried again. `TestAuthFailureSurvivesRemint`/`TestRemintErrorIsReportedAsAuthFailure` back this up. - Abort semantics (watch.go:110-113, cmd/watchpr/main.go:134-138): a 401/403 that survives re-mint aborts `Watch` non-zero; 5xx/network errors keep the old warn-and-continue path (`TestServerErrorDoesNotRemint`, `TestWatchContinuesPastServerError`). - Anonymous mode: `attempt` omits `Authorization` entirely when `Token==""` (gitea.go:430-433); `do` only calls `Refresh` when `IsAuthError(err)`, so a 200 never mints (`TestAnonymousPollingNeverMints`). Startup failure to mint is now a warning, not fatal, only in `watchpr`'s `clientFor` — `agentpr`'s `client()` (cmd/agentpr/main.go:47-53) is untouched and still fails fast, and `RefreshGiteaToken` re-mints from the same env-derived role/creds path, so no command can end up acting as a different identity than before. Two non-blocking design notes, since you asked me to reason about thundering-herd behaviour explicitly: - `RefreshGiteaToken` (internal/agent/token.go:109-115) has no single-flight/coalescing: it unconditionally re-mints on every call. The global mutex serializes concurrent callers (no data race, no lost update) but does *not* dedupe them — N concurrent 401s on a shared client would still cost N sequential Vault mints, not 1. Not reachable today: `Watch` drives one `*GiteaClient` from a single goroutine, and every `cmd/*` entrypoint does the same, so this is latent rather than a live bug. Worth a comment or a follow-up if a client is ever shared across goroutines. - Relatedly, `GiteaClient.Token` is mutated in `do()` (gitea.go:415) with no synchronization on the client itself — fine under the current one-goroutine-per-client usage, would race under concurrent use of the same client instance. Minor scope note: the PR bundles two related-but-distinct concerns — (1) mid-watch token expiry (remint/replay/abort) and (2) anonymous polling when no token can be minted at all (a startup-time fallback, previously fatal). Both are small and well tested, so I'm not requesting a split, just flagging it since (2) trades a fast-fail on Vault misconfiguration for silent-if-nobody-reads-stderr degraded operation on public repos. That tradeoff is intentional and documented (AGENTS.md, PR body), just worth being deliberate about. PR body is within the size/format convention (6 lines, 412 chars, why + present-tense bullets).

benvin merged commit 985b58c406 into main

2026-09-09 22:34:15 +10:00

benvin deleted branch benvin/watchpr-auth-expiry

2026-09-09 22:34:15 +10:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/agent-tools#10