Skip to content

Fix four worker review findings from #14 - #39

Closed
Aspirant200715 wants to merge 2 commits into
mainfrom
fix/worker-review-findings
Closed

Aspirant200715 wants to merge 2 commits into
mainfrom
fix/worker-review-findings

Conversation

@Aspirant200715

Copy link
Copy Markdown
Collaborator

Closes #31

What changed

  • Sandbox timeout: daemon-thread wait, 30s bound after the restart, SandboxError if the restart fails.
  • Nested .git scan: capped at 500,000 entries. Over the cap or on an OS error it raises WorkspaceError, never 'not found'.
  • Truncation marker names the applied cap. The default equals the literal in spec/receipt-v0.md.
  • SandboxError in tools.execute is a failed step. SandboxError in run_phi is inconclusive (beyond the issue's four items).

What I verified

  • Full control-plane suite: 529 passed (-m 'not gvisor'). ruff and pyright clean.
  • 23 new Docker-free regression tests. Reverting each guard makes at least one fail.

Not verified

  • Real Docker daemon wedging after a kill: simulated with a fake API only.
  • The 500,000-entry cap was not tried on a real repository that large.

- sandbox: bound the wait on a stuck exec stream after a timeout kill
- workspace: cap the nested .git scan and fail closed over the cap
- models: truncation marker names the cap actually applied
- tools, phi: SandboxError is one failed step / an inconclusive phi
_find_name kept every directory it found open until the scan ended, so a
checkout with about a thousand directories ran out of descriptors under
the usual 1024 soft limit. With the fail-closed handling in this PR, every
git step on such a checkout (diff, changed paths, the Phi path check) was
then refused. The scan now keeps only the directories on the current path
open.

@OfficialAbhinavSingh OfficialAbhinavSingh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

checked on the branch and merged with main, everything passes. the daemon thread swap and failing closed at the scan cap both look right.

one thing i pushed a fix for: _find_name kept an fd open for every directory until the scan ended, so under a 1024 soft limit a checkout with ~1000 dirs hit EMFILE, and with this PR that turns into every git step being refused. it now only keeps the current path open, test included.

small nit: spec/receipt-v0.md still says the marker is always 1048576, and _walk has the same fd pattern, can do that as a follow-up.

@Viscous106

Copy link
Copy Markdown
Member

Superseded by #41, which contains this and fixes _walk.

@Viscous106 Viscous106 closed this Nov 2, 2026
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Kanban Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Worker: four review findings from #14 still open after the fix pass

3 participants