Skip to content

feat(workflow): keep push credentials fresh across long ACP turns#568

Open
thewoolleyman wants to merge 1 commit into
fabro-sh:mainfrom
thewoolleyman:push-credential-refresh-ahead
Open

feat(workflow): keep push credentials fresh across long ACP turns#568
thewoolleyman wants to merge 1 commit into
fabro-sh:mainfrom
thewoolleyman:push-credential-refresh-ahead

Conversation

@thewoolleyman

Copy link
Copy Markdown
Contributor

Problem

GitHub App installation tokens expire ~60 minutes after minting. On a long
run, the origin token embedded in the sandbox clone at run start is stale by
the time a late ACP node (e.g. the PR node) runs git push, which fails with
Invalid username or token.

Change

Two host-driven mechanisms, both reusing the existing
Sandbox::refresh_push_credentials() (re-mint + git remote set-url origin)
over the exec channel — no new inbound surface:

  1. Turn-entry re-mint — refresh at each ACP node entry, so a push early in
    the turn uses a fresh token.
  2. Refresh-ahead loop — a background task, scoped to the turn by a
    drop-guard, that re-mints every 45 min so a single push-bearing turn that
    itself outlives the TTL stays fresh. A normal sub-interval turn never ticks;
    a failed/timed-out tick retries sooner so a transient error cannot leave a
    longer-than-interval expired-token window.

Both refresh calls are timeout-bounded (30s) so a stalled GitHub API cannot
hang node entry (ACP nodes use the HandlerManaged timeout policy, so nothing
else bounds this pre-spawn await).

Controls (both added to the worker env allowlist):

  • FABRO_PUSH_CRED_REFRESH_AHEAD — default on; a falsy value
    (empty/0/false/off/no, case-insensitive) disables the whole feature
    (turn-entry + loop) for operators who manage origin themselves.
  • FABRO_PUSH_CRED_REFRESH_INTERVAL_SECONDS — overrides the interval; 0
    disables just the loop.

refresh_push_credentials now returns RefreshOutcome (Refreshed /
Skipped) so callers log accurately: Refreshed only when a GitHub App token
was actually re-minted; a static PAT or pre-minted Installation token
short-circuits to Skipped before the set-url exec. (This also means a PAT
sandbox no longer re-runs set-url on refresh as it did before — intentional,
since only App installation tokens can be re-minted.)

Testing

  • clippy (-D warnings) and cargo fmt --check — clean.
  • New unit tests cover enable/interval parsing (default, 0-disable,
    case-insensitive falsy, empty, unparsable) and the no-credentials Skipped
    path.
  • Validated on real >60-min runs: a fresh mint was observed at +90 min with
    zero TTL-expiry errors; an A/B with the loop disabled reproduced the original
    Invalid username or token failure.

Known follow-ups (documented in-code)

  • Resumed/parked runs reconnect the sandbox with no GitHub App creds, so
    refresh no-ops for them until those creds are threaded through the reconnect
    path.
  • No freshness check on the turn-entry mint (it mints once per node entry even
    when the current token is fresh).
  • The background set-url can contend with the agent's own git on
    .git/config.lock; parallel ACP branches each run their own loop.
  • The refresh lives in the ACP handler only, though the stale-origin problem is
    stage-agnostic.
  • Refresh failures are logged but not surfaced as a RunNotice event.

GitHub App installation tokens expire ~60 min after minting. On a long run,
the origin token baked into the sandbox clone at clone time is stale by the
time a late ACP node (e.g. the PR node) runs `git push`, causing an
`Invalid username or token` failure.

Two host-driven mechanisms, both using the existing
`Sandbox::refresh_push_credentials()` (re-mint + `git remote set-url origin`)
over the exec channel — no new inbound surface:

1. Turn-entry re-mint at each ACP node entry, so a push early in the turn uses
   a fresh token.
2. A background refresh-ahead loop, scoped to the turn via a drop-guard, that
   re-mints every 45 min so a single push-bearing turn that itself exceeds the
   TTL stays fresh. A normal sub-interval turn never ticks; a failed/timed-out
   tick retries sooner so a transient error cannot leave a longer-than-interval
   expired-token window.

Both refresh calls are timeout-bounded (30s) so a stalled GitHub API cannot
hang node entry. FABRO_PUSH_CRED_REFRESH_AHEAD (default on; falsy = empty/0/
false/off/no, case-insensitive) disables the whole feature — turn-entry and
loop — for operators who manage `origin` themselves;
FABRO_PUSH_CRED_REFRESH_INTERVAL_SECONDS overrides the interval (0 disables
just the loop). Both are added to the worker env allowlist.

refresh_push_credentials now returns RefreshOutcome (Refreshed vs Skipped) so
callers log accurately: Refreshed only when a GitHub App installation token was
actually re-minted; a static PAT or pre-minted Installation token (nothing to
re-mint) short-circuits to Skipped before the set-url exec.

Known follow-ups documented in-code: (a) resumed runs reconnect without App
creds, so refresh no-ops until they are threaded through the reconnect path;
(b) no freshness check on the per-entry mint; (c) the background set-url can
contend with the agent's own git on .git/config.lock; (d) parallel ACP branches
each run their own loop; (e) the refresh lives in the ACP handler only though
the stale-origin problem is stage-agnostic (native/command stages are not
covered); (f) refresh failures are logged via tracing but not surfaced as a
RunNotice event.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@swerner

swerner commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Thanks @thewoolleyman! Digging more in to this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants