Skip to content

Fix for Guardian CI Tests stack overflow, applying Box to reduce stack pressure#17846

Merged
won-openai merged 3 commits into
mainfrom
fix-bazel-guardian
Apr 15, 2026
Merged

Fix for Guardian CI Tests stack overflow, applying Box to reduce stack pressure#17846
won-openai merged 3 commits into
mainfrom
fix-bazel-guardian

Conversation

@won-openai

@won-openai won-openai commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Issue

guardian_parallel_reviews_fork_from_last_committed_trunk_history was failing on Windows/Bazel with a stack overflow:

thread 'guardian::tests::guardian_parallel_reviews_fork_from_last_committed_trunk_history' has overflowed its stack

  • This problem was a stack-headroom problem

Solution

Reduced stack pressure in the guardian async path by boxing thin wrapper futures, and run the affected test on a dedicated 2 MiB thread stack.

Concretely:

  • added Box::pin(...) around thin async wrapper hops in the guardian review/delegate path
  • changed guardian_parallel_reviews_fork_from_last_committed_trunk_history to run inside an explicitly sized thread stack so it has enough headroom in low-stack environments

@won-openai won-openai enabled auto-merge (squash) April 14, 2026 22:25
@won-openai won-openai changed the title Fix for CI Tests failing from stack overflow Fix for CI Tests failing from stack overflow, applying Box to reduce stack pressure Apr 14, 2026
@won-openai won-openai changed the title Fix for CI Tests failing from stack overflow, applying Box to reduce stack pressure Fix for Guardian CI Tests stack overflow, applying Box to reduce stack pressure Apr 14, 2026
Comment on lines +1324 to +1332
let handle =
std::thread::Builder::new()
.name("guardian_parallel_reviews_fork_from_last_committed_trunk_history".to_string())
.stack_size(TEST_STACK_SIZE_BYTES)
.spawn(|| -> anyhow::Result<()> {
let runtime = tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()?;
runtime.block_on(Box::pin(async {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of this now that you have boxed things elsewhere?

@won-openai won-openai Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried removing and ci regressed, bazel local-build fails without the harness due to the same issue

@bolinfest

Copy link
Copy Markdown
Collaborator

Let's land as-is to get the build green, but this is a bad code smell to me, so unless you have good evidence that it is not a bad code smell, then please eliminate the stack carveout post-merge.

@bolinfest bolinfest self-requested a review April 15, 2026 01:04
@won-openai won-openai merged commit 2bfa627 into main Apr 15, 2026
32 of 36 checks passed
@won-openai won-openai deleted the fix-bazel-guardian branch April 15, 2026 01:04
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants