Address SDK review issues: context, type safety, and robustness#4
Merged
dmortondev merged 5 commits intomainfrom Mar 6, 2026
Merged
Address SDK review issues: context, type safety, and robustness#4dmortondev merged 5 commits intomainfrom
dmortondev merged 5 commits intomainfrom
Conversation
Non-breaking internal changes only; no public API modifications. - Replace goto poll with idiomatic for+select loops (5 locations) - Add panic recovery to cache background goroutine - Use non-blocking channel sends to prevent deadlocks under load - Fix file handle leak in logger when debug log creation fails - Close race window in RunningJobContext with post-subscribe recheck - Handle JSON unmarshal error in requestFromJSON instead of discarding - Add nil guards to stream ResponseWriter methods - Convert Prune to range-over-func syntax and remove stale TODOs
Breaking changes to improve API consistency: - Add context.Context as the first parameter to all non-streaming Plugin methods (SubmitJob, GetJob, GetJobs, ControlJob, GetJobNetwork, ClusterInfo), plus Bootstrap and GetClusters on extension interfaces. Streaming methods already accepted context. - Change cache package public methods (Lookup, Update, WriteJob, RunningJobContext, StreamJobStatus) to accept api.JobID instead of string, providing type safety at API boundaries. Internal store keeps string; conversions happen at the cache layer. - Document Error.Is() sentinel-style matching, ResponseWriter error return semantics, and WriteJob/WriteJobs coupling rationale. Update all implementations, conformance suite, examples, and docs.
Job.ID was left as `string` when api.JobID was introduced at the Plugin interface and cache boundaries (6e2b237). This created a friction point: plugin authors received api.JobID from interface methods but had to convert back and forth with Job.ID as string. Since api.JobID is a named string type, JSON serialization, literal assignments, and format verbs all work unchanged. The only new conversions are api.JobID(...) at ID generation sites and string(...) at the internal cache→store boundary. Simplifications: - Remove 6 api.JobID(job.ID) casts in cache, conformance, examples - Remove 2 api.JobID(id) casts where id was already typed New conversions: - api.JobID(fmt.Sprintf(...)) at 2 ID generation sites - string(job.ID) at 2 cache→store boundary calls Public API changes in conformance/ and plugintest/: - SubmitJob returns api.JobID (was string) - GetJob, ControlJob, WaitForStatus, WaitForTerminalStatus, CollectOutputStream accept api.JobID for id param - FindJobByID, AssertJobID, NewJobWithID, WithID accept api.JobID
- README.md: correct Go version requirement from 1.25 to 1.24 - CONTRIBUTING.md: fix nonexistent WithMemory() to WithLimit() - .gitignore: add built binaries in root and cmd/smoketest
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
context.Contextto all Plugin methods — non-streaming methods now accept context as their first parameter, aligning with Go conventions and enabling cancellation/timeout propagationapi.JobIDtype safety —Job.IDchanged fromstringtoapi.JobID, and cache/conformance/plugintest APIs updated to match, eliminating friction from back-and-forth conversionsgoto-poll loops with idiomaticfor+select, add panic recovery in cache goroutine, fix file handle leak in logger, close race window inRunningJobContext, handle previously-discarded JSON unmarshal errorsWithMemory()reference, update.gitignoreBreaking Changes
All changes are documented in CHANGELOG.md under
[Unreleased]:context.Contextadded as first param toSubmitJob,GetJob,GetJobs,ControlJob,GetJobNetwork,ClusterInfo,Bootstrap,GetClustersJob.IDtype changed fromstringtoapi.JobIDapi.JobIDinstead ofstringapi.JobIDTest plan
just cipasses (lint, fmt, test, build, build-examples)just test-racepasses with no data races