Skip to content

fix(app): snapshot the session before spawning background goroutines - #4230

Merged
aheritier merged 1 commit into
mainfrom
fix/app-session-snapshot
Sep 11, 2026
Merged

aheritier merged 1 commit into
mainfrom
fix/app-session-snapshot

Conversation

@aheritier

Copy link
Copy Markdown
Collaborator

Fixes #4229.

Why

go test -race -shuffle=on ./pkg/tui (the test-race job from #4221) still fails about one run in five on TestLoadSessionThenClickEditLabel after #4228. The race detector points at pkg/app: App.ReplaceSession writes a.session from the bubbletea update loop while the goroutine started by App.Start reads it to call EmitStartupInfo. reEmitStartupInfo and RunSkillFork hand a.session to their goroutines the same way. This is a production race, not a test artifact.

What

  • Start, reEmitStartupInfo and RunSkillFork now read a.session on the caller's goroutine and pass the value to the background work, the way Run / RunWithMessage already do. ReplaceSession re-emits startup info for the new session itself, so emitting for the snapshotted session is the intended behavior.
  • New TestAppBackgroundWorkSnapshotsSessionBeforeReplace drives each of the three entry points, calls ReplaceSession right after, and asserts both sessions reach the runtime. Without the fix it fails deterministically under -race (7 race reports across the three subtests).

Verification

  • go test -race -count=3 ./pkg/app: clean.
  • go test -race -count=10 -run TestLoadSessionThenClickEditLabel ./pkg/tui: clean (was ~1 in 5 failing).
  • task lint passes.

Independent of #4228; the two together make test-race green.

Start, reEmitStartupInfo and RunSkillFork read a.session from inside
the goroutines they spawn, which races with ReplaceSession writing the
field from the bubbletea loop. The race detector caught it about one run
in five in TestLoadSessionThenClickEditLabel, failing the test-race CI
job. Read the field on the caller's goroutine and hand the value to the
background work, as Run already does; ReplaceSession re-emits startup
info for the new session itself. Adds a regression test that covers all
three entry points and fails deterministically under -race without the
fix.

Fixes #4229
@aheritier
aheritier requested a review from a team as a code owner September 10, 2026 16:30
@aheritier aheritier added kind/fix PR fixes a bug (maps to fix:). Use on PRs only. status/needs-triage For issues that need to be triaged labels Sep 10, 2026
@aheritier
aheritier enabled auto-merge September 10, 2026 16:32
@aheritier aheritier added the area/core Core agent runtime, session management label Sep 10, 2026
@aheritier
aheritier added this pull request to the merge queue Sep 11, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 11, 2026
@aheritier
aheritier added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit e853b6e Sep 11, 2026
16 checks passed
@aheritier
aheritier deleted the fix/app-session-snapshot branch September 11, 2026 08:25
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
docker-agent 1.138.1

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.<details>
  <summary>release notes</summary>
  <pre>This release adds kubectl and AWS CLI to sandbox templates, introduces secure HTTP relay packages and improved share signing, fixes session and race condition bugs, and includes several CI pipeline improvements.

## What's New
- Adds `kubectl` and AWS CLI to the `sbx-templates` image, making them available in sandboxes without manual installation
- Adds secure HTTP relay packages (`pkg/config/httprelay` and `pkg/modelsgateway/relay`) for agent configuration fetching and model-API forwarding
- Changes `share push --key` to sign a DSSE-wrapped in-toto statement instead of raw YAML bytes, adding metadata about where and when the artifact was published

## Bug Fixes
- Fixes the starred flag being silently dropped when forking a starred session (the `AddSession` insert omitted the `starred` column)
- Fixes a production data race in `App` where `ReplaceSession` wrote `a.session` from the update loop while background goroutines started by `App.Start` read it concurrently
- Fixes a data race in evaluation tests where container stderr was read without going through `exec.Cmd`
- Fixes HCL examples not being decoded through the HCL loader during the live models check in `TestExamplesAgainstLiveModelsDev`

## Technical Changes
- Consolidates CI workflows: single gate job, merged image job, shared composite actions, merged docs workflows, shared module/build caches, and checksum-verified tools
- Adds race detector job (`go test -race -shuffle=on`) on pushes to `main`
- Skips Go CI jobs on docs-only PRs and skips redundant lint/license checks on main push (already verified in merge queue)
- Stops sidebar tests from reading the checkout's git branch to avoid environment-dependent failures in CI
- Tears down bubbletea programs in `t.Cleanup` and freezes animation clock in exact-frame TUI tests to eliminate test flakiness
- Stops parallel tests from mutating `version.Version` to prevent race conditions
---

## What's Changed
* docs: update CHANGELOG.md for v1.138.0 by @docker-read-write[bot] in docker/docker-agent#4226
* fix(session): preserve the starred flag in AddSession by @kitadesign in docker/docker-agent#4182
* ci: single gate, merged image job, shared composite actions, docs workflow merge by @aheritier in docker/docker-agent#4221
* test(tui): stop sidebar tests from reading the checkout's git branch by @aheritier in docker/docker-agent#4231
* ci: fix the failures in the new test-race job by @aheritier in docker/docker-agent#4228
* fix(app): snapshot the session before spawning background goroutines by @aheritier in docker/docker-agent#4230
* docs: auto-update for merged PRs (2026-09-11) by @aheritier in docker/docker-agent#4234
* feat: add secure HTTP relay packages by @dgageot in docker/docker-agent#4235
* fix(test): load HCL examples in live models check by @aheritier in docker/docker-agent#4236
* ci: skip redundant main push checks by @aheritier in docker/docker-agent#4237
* feat(sandbox): add kubectl and aws cli to sbx-templates image by @maxcleme in docker/docker-agent#4218
* feat(share): sign a DSSE-wrapped in-toto statement, not just the YAML bytes by @gtardif in docker/docker-agent#4238

## New Contributors
* @kitadesign made their first contribution in docker/docker-agent#4182

**Full Changelog**: docker/docker-agent@v1.138.0...v1.138.1
</pre>
  <p>View the full release notes at <a href="https://github.com/docker/docker-agent/releases/tag/v1.138.1">https://github.com/docker/docker-agent/releases/tag/v1.138.1</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!19945
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Core agent runtime, session management kind/fix PR fixes a bug (maps to fix:). Use on PRs only. status/needs-triage For issues that need to be triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data race on App.session between Start's startup-info goroutine and ReplaceSession

2 participants