Repository navigation
π agent: keep sandbox run persistence off the manager lock and bounded - #14864
Conversation
β¦nded Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
hive/awaiting-ci: CI pending β sweep will merge when green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Read runSandboxKick (src/crux/agent/manager_sandbox.go:203-275), persistSandboxRun/compactSandboxRunHistory (src/crux/agent/sandbox_run.go:201-257) and the only reader of runs.jsonl, pkg/tokens/sandbox_scanner.go:48-80. The change does what the body says; one consequence is worth noting for the operator.
Verified:
- Defer order is correct:
defer m.mu.Unlock()is registered after the persist defer (manager_sandbox.go:210,218), so LIFO runsUnlockfirst and the disk write happens unlocked.entryis fully built beforepending = &entry(:237-238) and nothing after that line mutatesrun, so the persisted record is identical to what was written before. persistSandboxRunis the only writer ofruns.jsonl(grep:sandbox_run.go:220is the soleOpenFileonSandboxRunHistoryPath), so the new package-levelsandboxRunPersistMufully serializes append+compact. No path takessandboxRunPersistMuand thenm.mu, so no lock-order inversion.ScanSandboxRunsopens andbufio.Scanners the whole file on every scan with no stored offset, so the rename-based compaction cannot leave it mid-line; it just sees fewer records.- All three closed issues are addressed: the
sandbox-isolation.md:129cross-link (#14853) targets an existing## Health and metrics for sandboxed agentsheading at:175.
Info (intent-alignment): because ScanSandboxRuns is the source for sandboxed-agent token attribution, compaction turns those totals into a sliding window of the newest 1000 runs β once a hive passes ~2 MiB of history, lifetime sandbox token figures on the dashboard will stop growing/decrease. That is implied by bounding the file and seems like the right trade for the scan-cost fix, but it is a visible behaviour change the body does not call out; a sentence in the changelog fragment or sandbox-isolation.md:179 would save an operator a puzzled moment.
Looks correct to me.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Persisting a sandbox run did disk I/O (mkdir, write, rename, append) while holding the manager mutex, so a slow data volume stalled every status and kick call. The write now happens after the lock is released, serialized by its own mutex.
runs.jsonl was append-only and the token collector re-parsed all of it on every scan, so scan cost grew with hive age. It is now compacted to its newest 1000 records once it passes 2 MiB; the format and paths are unchanged.
The lifecycle sentence in sandbox-isolation.md predated the Health section; it now points to it.
Closes #14853, Closes #14854, Closes #14855
β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88