Skip to content

fix(core): close contract edges in git/repomanager/workspace/targethasher (audit #14) - #262

Closed
sbalabanov wants to merge 1 commit into
mainfrom
audit/fix-14-contract-edges
Closed

fix(core): close contract edges in git/repomanager/workspace/targethasher (audit #14)#262
sbalabanov wants to merge 1 commit into
mainfrom
audit/fix-14-contract-edges

Conversation

@sbalabanov

Copy link
Copy Markdown
Contributor

Summary

Four methods on git.Interface (RevParse, IsAncestor, GetCommitTimeSecond, FileHashes) bypassed the timeout and wrapError path, so fatal git exits never surfaced as ErrFatal and the orchestrator could not classify them. Additionally, repomanager.Lease only wrapped ErrPoolTimeout for deadline exceeded (not plain cancellation), workspace.Release was not idempotent (double-release corrupted the pool), and targethasher's directory walk ignored context cancellation.

  • (a) git wrapping: Route all four methods through _gitTimeout + wrapError, preserving IsAncestor's exit-code-1 → (false, nil) semantics.
  • (b) ErrPoolTimeout: Wrap ErrPoolTimeout on both context.Canceled and context.DeadlineExceeded, matching the sentinel's godoc.
  • (c) Release idempotency: Guard workspace.Release with sync.Once so the onRelease callback fires exactly once.
  • (d) targethasher cancellation: Thread context.Context into hashDir and check ctx.Err() every cancelCheckInterval files.

Test plan

  • TestFatalExitCode_wrapsErrFatal — table-driven: fatal exit 128 from each of the four methods yields errors.Is(err, ErrFatal)
  • TestLease_CtxCanceled — updated: plain cancellation now asserts errors.Is(err, ErrPoolTimeout)
  • TestWorkspace_Release_Idempotent — channel-based: two Release calls, assert callback channel has exactly one entry
  • TestHashDir_RespectsCtxCancellation — pre-cancelled ctx with >cancelCheckInterval files, assert errors.Is(err, context.Canceled)
  • All existing tests pass (go test ./core/... ./orchestrator/ ./controller/ ./graphrunner/ ./mapper/)

🤖 Generated with Claude Code

(a) Route RevParse, IsAncestor, GetCommitTimeSecond, and FileHashes
through the same timeout+wrapError path as the other git.Interface
methods so fatal exits (128/129) surface as errors.Is(err, ErrFatal)
and the orchestrator's classifyGitError can tag them.

(b) Wrap ErrPoolTimeout on both context.Canceled and
context.DeadlineExceeded in repomanager.Lease, matching the godoc
contract.

(c) Make workspace.Release idempotent via sync.Once so double-release
cannot push the same slot back into the pool twice.

(d) Thread context.Context into targethasher's hashDir walk and check
ctx.Err() every cancelCheckInterval files so large-tree hashing aborts
promptly on request cancellation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

This draft has been split into three independent, focused replacement PRs based on the latest main:

The repomanager cancellation reclassification from this draft was intentionally not carried forward because mapping plain context.Canceled to ErrPoolTimeout would also change orchestrator retry semantics. That behavior should be proposed separately if desired.

Closing #262 as superseded by the PRs above.

@sbalabanov sbalabanov closed this Aug 3, 2026
sbalabanov pushed a commit that referenced this pull request Aug 3, 2026
.1)

Summary:
Audit #14.1 extracts the git command contract fixes from #262.

Intent:
- Ensure read-only git commands honor the same timeout and error-classification contract as mutating commands.
- Preserve conditional exit semantics for merge-base ancestry checks.

Changes:
- Apply the shared timeout and wrapError path to RevParse, IsAncestor, GetCommitTimeSecond, and FileHashes.
- Cover fatal exits, deadline expiration, parent cancellation, and IsAncestor exit code 1.
sbalabanov pushed a commit that referenced this pull request Aug 3, 2026
….3)

Summary:
Audit #14.3 extracts targethasher cancellation propagation from #262.

Intent:
- Stop directory hashing promptly when graph computation is canceled.
- Preserve cancellation causes through both target graph implementations.

Changes:
- Thread context through SourceHasher and all callers.
- Check cancellation before and periodically during directory traversal.
- Cover public hasher cancellation and context propagation through both graph paths.
sbalabanov pushed a commit that referenced this pull request Aug 4, 2026
Summary:
Audit #14.2 extracts workspace release safety from #262.

Intent:
- Prevent repeated releases from returning the same workspace slot more than once.
- Make the lifecycle contract safe under concurrent cleanup paths.

Changes:
- Guard the release callback with sync.Once.
- Document the idempotent Release contract.
- Cover sequential and concurrent repeated releases.
sbalabanov pushed a commit that referenced this pull request Aug 4, 2026
….3)

Summary:
Audit #14.3 extracts targethasher cancellation propagation from #262.

Intent:
- Stop directory hashing promptly when graph computation is canceled.
- Preserve cancellation causes through both target graph implementations.

Changes:
- Thread context through SourceHasher and all callers.
- Check cancellation before and periodically during directory traversal.
- Cover public hasher cancellation and context propagation through both graph paths.
sbalabanov pushed a commit that referenced this pull request Aug 5, 2026
.1)

Summary:
Audit #14.1 extracts the git command contract fixes from #262.

Intent:
- Ensure read-only git commands honor the same timeout and error-classification contract as mutating commands.
- Preserve conditional exit semantics for merge-base ancestry checks.

Changes:
- Apply the shared timeout and wrapError path to RevParse, IsAncestor, GetCommitTimeSecond, and FileHashes.
- Cover fatal exits, deadline expiration, parent cancellation, and IsAncestor exit code 1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants