[AGE-4000] fix(runner): rebuild agent mounts before STS credentials expire - #5520
[AGE-4000] fix(runner): rebuild agent mounts before STS credentials expire#5520mmabrouk wants to merge 5 commits into
Conversation
The web-identity JWT was hardcoded to 15 minutes, so SeaweedFS capped every mount credential at 900s regardless of the requested 3600s. Mint the token with the requested lifetime (capped at the 12h session ceiling), clamp DurationSeconds into each backend's valid range, make the TTL configurable via AGENTA_MOUNTS_CREDENTIALS_TTL_SECONDS, and raise instead of returning never-expiring credentials when an STS response carries no usable Expiration. Claude-Session: https://claude.ai/code/session_01BKPLpy1rUTBj5XktkP5eKY
…y die The session pool re-armed its expiry bookkeeping from each dispatch's fresh sign, which never reaches the running geesefs daemons, so active conversations reused mounts whose credentials had died (EACCES on every path, issue #5516). Record the expiry of the credentials actually installed at mount time, park from that installed lease, and evict to a cold rebuild when the lease cannot cover a worst-case turn plus clock skew. Claude-Session: https://claude.ai/code/session_01BKPLpy1rUTBj5XktkP5eKY
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change makes mount STS credential lifetimes configurable and backend-valid, rejects unusable STS expirations, and tracks installed mount leases in the runner to control session reuse and remount decisions. ChangesSTS credential lifetime and parsing
Runner lease tracking and keep-alive enforcement
Design and deployment documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Runner
participant MountsService
participant STS
participant SessionPool
Runner->>MountsService: request mount credentials
MountsService->>STS: request bounded session duration
STS-->>MountsService: credentials and Expiration
MountsService-->>Runner: mount credentials
Runner->>SessionPool: record installed mount lease
SessionPool-->>Runner: reuse environment or evict before required validity deadline
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Inline notes to speed up the review. Each one states the invariant or the reason behind the nearby change; decisions.md in the design folder has the full register with trade-offs and how to back each choice out.
|
|
||
| # Lifetime of signed mount credentials, in seconds. The store backends clamp it to their | ||
| # own STS bounds. | ||
| credentials_ttl_seconds: int = Field( |
There was a problem hiding this comment.
The default_factory makes the env var get read when the config object is built, not at module import, so tests can construct MountsConfig() under monkeypatch without reloading the module. Parsing goes through the file's positive-int helper: unset or empty falls back to 3600; garbage or a non-positive value raises at startup, same as the file's other knobs.
| # min(DurationSeconds, web-identity token expiry, maxSessionLength). The token-expiry cap | ||
| # has no floor, so minting the token at the capped TTL is what lets a sub-900 TTL take | ||
| # effect (the QA lever); DurationSeconds only has to stay inside the validated range. | ||
| capped = min(duration_seconds, _SEAWEEDFS_MAX_SESSION_SECONDS) |
There was a problem hiding this comment.
The core of the SeaweedFS half: the token now carries the real requested lifetime (capped at the 12h session ceiling), while DurationSeconds only needs to pass SeaweedFS's [900, 43200] validation. The effective lifetime is min(DurationSeconds, token expiry, maxSessionLength), and the token-expiry cap has no floor, which is what lets QA run 120-second credentials that AWS's hard 900 floor would refuse.
| except ValueError: | ||
| expiration = None | ||
| if not raw_exp: | ||
| raise MountStorageUnavailable("STS response carried no credential expiry.") |
There was a problem hiding this comment.
Fail closed on purpose: the runner reads a missing expiry as "never expires", so one malformed STS response used to disable the reuse bound silently for the lifetime of a pooled environment. A refused sign degrades the turn to mount-less (the existing handling for any sign failure); it does not fail the turn. Both signing branches are STS, so there is no legitimate no-expiry path.
| * The Daytona harness session-dir mounts are deliberately excluded: they are signed after the | ||
| * cwd mount with the same TTL, so their expiry is never the minimum. | ||
| */ | ||
| installedMountExpiries: InstalledMountExpiries; |
There was a problem hiding this comment.
This field is the heart of the runner half: the expiry of the credentials the running daemons actually hold, stamped only where a mount succeeds. The Daytona harness session-dir mounts are deliberately excluded; they are signed after the cwd mount with the same TTL, so their expiry can never be the minimum. If that ordering ever changes, they need entries here (decisions.md item 7).
| * exactly on `asOfMs` counts as spent. The one place the lease comparison lives, so "already dead" | ||
| * and "dead before the turn ends" can never drift apart. | ||
| */ | ||
| export function leaseExpiresBy( |
There was a problem hiding this comment.
One primitive owns the boundary semantics (<=, and undefined lease means no bound). Both the warm-path eviction and the cold-path lease-short warning delegate here, so they cannot drift apart at the edge. credentials-expired vs credentials-expiring is purely a log-label distinction for operators reading evictions.
| const incomingEpoch = computeCredentialEpoch(request); | ||
| // Resolved once per dispatch: the longest a turn started now could still be running. | ||
| const turnBudgetMs = resolveRunLimits().totalMs; | ||
| const requiredValidThroughMs = |
There was a problem hiding this comment.
Resolved once per dispatch: the lease must cover the longest a turn started now could still be running (45 min default) plus 60s of fixed clock skew. Consequence worth knowing: at default knobs the warm-reuse window is TTL 3600 minus 2700 minus 60, about 14 minutes, so a continuously active conversation rebuilds cold roughly every 14 minutes. That replaces breaking outright at minute 15; the follow-up if churn hurts is in-place refresh, not a longer TTL (decisions.md item 2).
|
|
||
| // A parked epoch's expiry comes from the credentials installed in the environment's mounts, not | ||
| // from whatever this dispatch signed to compute the pool key. | ||
| const parkedEpoch = ( |
There was a problem hiding this comment.
Both park sites go through this helper so the rule lives in one place: a parked epoch's expiry always comes from the mounts actually installed, never from the credentials this dispatch signed (those never reach the daemons on a warm turn, which was defect 2 of the bug).
| configFingerprint: cfgFp, | ||
| historyFingerprint: nextHistoryFp(env), | ||
| credentialEpoch: incomingEpoch, | ||
| credentialEpoch: parkedEpoch(env, live.credentialEpoch.secretsHash), |
There was a problem hiding this comment.
Always the live environment's hash, unconditionally: on the idle path the mismatch gate a few lines up already proved live and incoming hashes equal, and the approval-resume path deliberately ignores the incoming credentials, so adopting the incoming hash there would relabel an environment running old secrets as if it ran new ones. An earlier draft made this a per-call-site flag; the unconditional form removes a default that would be wrong to forget.
| if (leaseExpiresBy(leaseMs, requiredValidThroughMs)) { | ||
| // A misconfigured TTL must never fail a turn: warn once per acquisition and run it. | ||
| klog( | ||
| `lease-short key=${key} leaseExpiresAtMs=${leaseMs} ` + |
There was a problem hiding this comment.
A TTL below the turn budget plus skew is not an error by design: every dispatch rebuilds cold (correct, denial-free, churny), the log names both knobs, and QA lowers the TTL on purpose. A hard failure here would turn one mis-set number into a full outage; proceeding is never worse than the pre-fix behavior (decisions.md item 6).
| // | ||
| // A parked environment runs on the credentials its geesefs daemons were handed at mount time, not | ||
| // on the ones each dispatch signs to compute the pool key. These cases pin that the pool parks the | ||
| // installed lease and evicts to a cold rebuild before a worst-case turn could outlive it. |
There was a problem hiding this comment.
The test that pins the bug is "a warm repark keeps the installed lease": turn 1 mounts with a short expiry, turn 2 signs a far-future one and runs warm, then advancing the fake clock past the installed expiry must force turn 3 cold. On the pre-fix code turn 3 warm-continues onto the dead mount, which is exactly the field failure. Fake Date only; the pool's real timers stay live.
Live QA evidence (SeaweedFS, dedicated stack from this branch)All timestamps UTC, 2026-07-25. Full logs and the wire captures are archived on the dev box under Phase A: bug reproduced on demand (pre-fix runner + this branch's API, TTL=120s). Mount signed 19:32:14 with Bonus finding: turn 7, started exactly at the 120s boundary, reported a successful append; that line is absent from the file afterward. The pre-fix behavior can silently lose an acknowledged write in the geesefs cache flush, not just deny access. Phase B: fixed runner, identical cadence. At the exact wall-clock offset where phase A first denied (+143.8s), the fixed runner's turn succeeded. 18/18 turns clean, file grew monotonically 9 to 26 lines across every rebuild, lease-short warning (TTL=60 < 30s budget + 60s skew): every acquisition logged the warning naming both knobs; turns still succeeded. Defaults soak (knobs removed, 22 minutes, turns every 60s): mount signed 19:47:30 with A screen recording of the reproduction and the fix is coming as a follow-up comment. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Railway Preview Environment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dfb3dfd-e7f7-4b8b-b308-d8fee5256a79
📒 Files selected for processing (26)
api/oss/src/core/mounts/service.pyapi/oss/src/core/store/storage.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/test_mounts_injection.pydocs/design/fix-sts-mount-expiry/README.mddocs/design/fix-sts-mount-expiry/context.mddocs/design/fix-sts-mount-expiry/decisions.mddocs/design/fix-sts-mount-expiry/plan.mddocs/design/fix-sts-mount-expiry/research.mddocs/design/fix-sts-mount-expiry/status.mdhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.local.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.local.ymlhosting/docker-compose/oss/docker-compose.gh.ssl.ymlhosting/docker-compose/oss/docker-compose.gh.ymlservices/runner/src/engines/sandbox_agent/environment-setup.tsservices/runner/src/engines/sandbox_agent/environment.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/engines/sandbox_agent/session-identity.tsservices/runner/src/server.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/utils/capture-stderr.ts
| ## 7. The lease tracks the cwd and agent mounts only | ||
|
|
||
| `installedMountExpiries` records the two durable mounts. The Daytona harness | ||
| session-directory and transcript mounts are excluded: they are signed seconds | ||
| after the cwd mount with the same TTL, so their expiry is never the minimum and | ||
| tracking them would add entries that cannot change the outcome. | ||
|
|
||
| - Trade-off: if that signing order or TTL parity ever changes, the exclusion | ||
| argument breaks silently. | ||
| - Backtrack: add fields to `InstalledMountExpiries` and stamp the two remote | ||
| session-dir mount sites; `installedMountLease` already reduces over whatever | ||
| entries exist. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the tracked mounts accurately.
installedMountExpiries tracks the session cwd mount and the durable agent mount; cwd is not itself durable. Calling both “the two durable mounts” contradicts README.md and may confuse future lease changes.
Suggested wording
-`installedMountExpiries` records the two durable mounts.
+`installedMountExpiries` records the two tracked mounts: the session `cwd` and durable agent mounts.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 7. The lease tracks the cwd and agent mounts only | |
| `installedMountExpiries` records the two durable mounts. The Daytona harness | |
| session-directory and transcript mounts are excluded: they are signed seconds | |
| after the cwd mount with the same TTL, so their expiry is never the minimum and | |
| tracking them would add entries that cannot change the outcome. | |
| - Trade-off: if that signing order or TTL parity ever changes, the exclusion | |
| argument breaks silently. | |
| - Backtrack: add fields to `InstalledMountExpiries` and stamp the two remote | |
| session-dir mount sites; `installedMountLease` already reduces over whatever | |
| entries exist. | |
| ## 7. The lease tracks the cwd and agent mounts only | |
| `installedMountExpiries` records the two tracked mounts: the session `cwd` | |
| and durable agent mounts. The Daytona harness session-directory and transcript | |
| mounts are excluded: they are signed seconds after the cwd mount with the same | |
| TTL, so their expiry is never the minimum and tracking them would add entries | |
| that cannot change the outcome. | |
| - Trade-off: if that signing order or TTL parity ever changes, the exclusion | |
| argument breaks silently. | |
| - Backtrack: add fields to `InstalledMountExpiries` and stamp the two remote | |
| session-dir mount sites; `installedMountLease` already reduces over whatever | |
| entries exist. |
| Add the TTL, following the existing `int(os.getenv(...) or "...")` pattern | ||
| (compare `catalog_cache_ttl_seconds`, env.py:622). The `or` also absorbs an empty | ||
| string, which is what an unset compose passthrough delivers: | ||
|
|
||
| ```python | ||
| class MountsConfig(BaseModel): | ||
| """Mounts-domain config. Store credentials live in StoreConfig.""" | ||
|
|
||
| # Lifetime of signed mount credentials. The store backends clamp it to their own | ||
| # STS bounds (SeaweedFS: effective floor via the web-identity token, hard range | ||
| # 900-43200 for DurationSeconds; AWS GetFederationToken: 900-129600). QA lowers | ||
| # it to force fast expiry; only SeaweedFS honors values below 900. | ||
| credentials_ttl_seconds: int = int( | ||
| os.getenv("AGENTA_MOUNTS_CREDENTIALS_TTL_SECONDS") or "3600" | ||
| ) | ||
|
|
||
| model_config = ConfigDict(extra="ignore") | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the design records with the final implementation.
The plan and research documents mix historical pre-fix behavior with current-tree claims and stale code samples. Either update these sections to the implemented contracts or explicitly label them as pre-fix/historical.
docs/design/fix-sts-mount-expiry/plan.md#L55-L72: document the actualdefault_factory/positive-integer parser rather than the obsolete rawint(os.getenv(...))implementation.docs/design/fix-sts-mount-expiry/plan.md#L328-L360: remove the obsoletekeepSecretsHashoption; the final design keeps the live environment’s hash unconditionally.docs/design/fix-sts-mount-expiry/plan.md#L454-L461: replace undefinedrequiredMsandtotalMsplaceholders with the actual names.docs/design/fix-sts-mount-expiry/plan.md#L619-L626: close the transcript-mount question, sincedecisions.mdandstatus.mdrecord that it was resolved.docs/design/fix-sts-mount-expiry/research.md#L3-L7: revise “current tree” or update the referenced locations.docs/design/fix-sts-mount-expiry/research.md#L17-L53: mark the removed TTL constant, old duration clamp, and optional-expiry behavior as pre-fix.docs/design/fix-sts-mount-expiry/research.md#L103-L131: mark the old dispatch-expiry bookkeeping as historical.
📍 Affects 2 files
docs/design/fix-sts-mount-expiry/plan.md#L55-L72(this comment)docs/design/fix-sts-mount-expiry/plan.md#L328-L360docs/design/fix-sts-mount-expiry/plan.md#L454-L461docs/design/fix-sts-mount-expiry/plan.md#L619-L626docs/design/fix-sts-mount-expiry/research.md#L3-L7docs/design/fix-sts-mount-expiry/research.md#L17-L53docs/design/fix-sts-mount-expiry/research.md#L103-L131
| empty string falls back to 3600 (construct `MountsConfig` under | ||
| `monkeypatch.setenv`). | ||
|
|
||
| Run: `cd api && uv run --no-sync pytest oss/tests/pytest/unit/test_mounts_injection.py`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the repository-mandated API test command.
The plan documents uv run --no-sync pytest, but the repository requires API tests to run with py-run-tests from the api directory. Update this command so the validation step matches project tooling.
As per coding guidelines, api/**/*.py: Run API tests with py-run-tests from the api directory.
Source: Coding guidelines
|
QA screen recording (2 min 40 s): the bug reproduced live with 120-second credentials (turns 7-11 all "Permission denied", first denial 3 seconds after expiry, the runner warm-reusing the dead mount), then the fixed runner on the identical cadence: 11/11 turns clean, 10 credentials-expiring rebuilds, nothing lost. qa-5516-repro-and-fix.mp4 |
1 participant
Context
Fixes #5516. Any agent conversation that stayed active past 15 minutes hit a wall: every file access under the agent's mounts returned "Permission denied" for minutes at a time (16 windows in one day on the OSS deployment, the longest 14 minutes), interrupted writers left stale git lock files, and the live QA reproduction showed a write acknowledged at the boundary being silently lost.
Two defects combined to cause this, and neither is the one the issue title suggests:
Changes
API (
acaa86e288): the web-identity token is minted with the requested lifetime (capped at the 12-hour session ceiling),DurationSecondsis clamped into each backend's valid range, the TTL becomesAGENTA_MOUNTS_CREDENTIALS_TTL_SECONDS(default 3600, wired through all seven compose files), and signing now raises instead of returning never-expiring credentials when an STS response carries no usableExpiration.Before: requested 3600s, effective lifetime 900s (token cap), and a malformed expiry silently meant "never expires".
After: requested 3600s, effective lifetime 3600s, and a missing expiry refuses the sign (the turn degrades to mount-less, same as any sign failure).
Runner (
290cfc244b): the environment records the expiry of the credentials actually installed in each geesefs daemon at mount time (installedMountExpiries), both park paths derive the pool's expiry from that installed lease, and reuse requires the lease to cover a full worst-case turn plus 60 seconds of clock skew. When it cannot, the dispatch logsmismatch (credentials-expiring)and rebuilds cold at the turn boundary, before any writer can be interrupted.Before: pool expiry = whatever the latest dispatch signed (never installed anywhere).
After: pool expiry = min expiry of the mounts actually running; no turn starts on credentials it could outlive.
No wire changes, no new daemons, no refresh machinery inside the sandbox. A TTL misconfigured below the turn budget warns (
lease-short) and runs rather than failing; the worst case equals today's behavior.Why this is safe on both self-hosted SeaweedFS and real AWS S3
The runner never inspects which backend signed its credentials; it keys off the STS-reported
expires_at, which both backends report truthfully and enforce with 403. On SeaweedFS the token-lifetime fix is what makes the TTL real. On AWS there is no web-identity path, so that change is inert; the TTL passes throughGetFederationTokenwith AWS's own [900, 129600] bounds. The full two-backend analysis, including the AWS constraint that signing requires long-term IAM-user credentials, is indocs/design/fix-sts-mount-expiry/research.mdsection 4.Design docs
The planning workspace ships in this PR under
docs/design/fix-sts-mount-expiry/:context.md(the problem),research.md(credential flow end to end, the geesefs v0.43.0 refresh-mechanism analysis, the SeaweedFS-vs-AWS section),plan.md(the fix and QA plan),status.md, anddecisions.md, a register of every decision with its trade-off and how to back it out. Start withdecisions.mdif you want to challenge choices rather than read code.Tests
pnpm test1209 tests green,pnpm run typecheckclean. New vitest cases pin the bug deterministically with fake clocks: a warm repark keeps the installed lease (fails without the fix), approval resume preserves the live secrets hash and lease, the lease is the minimum across mounts, and boundary cases at exactly/past the required-validity window.pytestunit suite 1403 green. New request-level tests capture the actual STS POST and assertDurationSecondsclamping and the token'sexp - iatfor TTLs 120/3600/90000, plus fail-closed on missing and unparsableExpiration.What to QA
AGENTA_MOUNTS_CREDENTIALS_TTL_SECONDS=120on the api service andAGENTA_RUNNER_RUN_TOTAL_TIMEOUT_MS=30000on the runner, chat with an agent that appends to a file inagent-files/every ~20 seconds. Expect periodicmismatch (credentials-expiring)cold rebuilds in the runner log and never a "Permission denied".Known trade-off to weigh in review
With default knobs, a continuously active conversation now rebuilds cold about every 14 minutes (TTL 3600 minus the 45-minute worst-case turn budget minus skew). That replaces the previous outcome, which was breaking outright at minute 15. If production shows the churn matters, the designated follow-up is in-place refresh via a container-credentials endpoint (
decisions.mditem 2), not a longer TTL.https://claude.ai/code/session_01BKPLpy1rUTBj5XktkP5eKY