Merge pull request #4 from posit-dev/address-review-issues

Address SDK review issues: context, type safety, and robustness

Author: dmortondev

.gitignore

@@ -30,6 +30,11 @@ go.work
 examples/inmemory/inmemory
 examples/inmemory/inmemory-plugin
 
+# Built binaries in root
+/inmemory
+/smoketest
+cmd/smoketest/smoketest
+
 # Temporary files
 *.tmp
 *.log

CHANGELOG.md

@@ -9,6 +9,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 - **BREAKING**: `cache.NewJobCache` no longer accepts a `dir` parameter. The SDK now defaults to in-memory caching, which aligns with how Launcher plugins are expected to work: the scheduler owns job state, and plugins populate the cache during `Bootstrap()` and keep it in sync via periodic polling.
+- **BREAKING**: Add `context.Context` as the first parameter to all non-streaming `Plugin` methods (`SubmitJob`, `GetJob`, `GetJobs`, `ControlJob`, `GetJobNetwork`, `ClusterInfo`) and extension interfaces (`Bootstrap`, `GetClusters`). Streaming methods already accepted context.
+- **BREAKING**: `Job.ID` type changed from `string` to `api.JobID` for end-to-end type safety. Since `api.JobID` is a named `string` type, JSON serialization and literal assignments work unchanged.
+- **BREAKING**: Cache public methods (`Lookup`, `Update`, `WriteJob`, `RunningJobContext`, `StreamJobStatus`) now accept `api.JobID` instead of `string`.
+- **BREAKING**: Conformance and plugintest helpers updated to use `api.JobID` (`SubmitJob` returns `api.JobID`; `GetJob`, `ControlJob`, `WaitForStatus`, `FindJobByID`, `AssertJobID`, `NewJobWithID`, `WithID` accept `api.JobID`).
+- Replace `goto`-based poll loops with idiomatic `for`+`select` loops in cache and protocol packages
+- Add panic recovery to cache background goroutine
+- Use non-blocking channel sends to prevent deadlocks under load
+- Add nil guards to stream `ResponseWriter` methods
+- Convert `Prune` to range-over-func syntax
+
+### Fixed
+- File handle leak in logger when debug log creation fails
+- Race window in `RunningJobContext` with post-subscribe recheck
+- JSON unmarshal error in `requestFromJSON` now handled instead of silently discarded
+- Go version requirement corrected from 1.25 to 1.24 in README
+- `WithMemory()` reference corrected to `WithLimit()` in CONTRIBUTING.md
 
 ### Removed
 - BoltDB (`go.etcd.io/bbolt`) dependency — in-memory caching is now the standard approach

CONTRIBUTING.md

@@ -298,7 +298,7 @@ func ExampleJobBuilder() {
     job := plugintest.NewJob().
         WithUser("alice").
         WithCommand("python train.py").
-        WithMemory("8GB").
+        WithLimit("memory", "8GB").
         Build()
 
     fmt.Println(job.User)

README.md

@@ -56,8 +56,8 @@ type MyPlugin struct {
     nextID int32
 }
 
-func (p *MyPlugin) SubmitJob(w launcher.ResponseWriter, user string, job *api.Job) {
-    job.ID = fmt.Sprintf("job-%d", atomic.AddInt32(&p.nextID, 1))
+func (p *MyPlugin) SubmitJob(ctx context.Context, w launcher.ResponseWriter, user string, job *api.Job) {
+    job.ID = api.JobID(fmt.Sprintf("job-%d", atomic.AddInt32(&p.nextID, 1)))
     job.Status = api.StatusPending
     if err := p.cache.AddOrUpdate(job); err != nil {
         w.WriteError(err)
@@ -66,8 +66,8 @@ func (p *MyPlugin) SubmitJob(w launcher.ResponseWriter, user string, job *api.Jo
     p.cache.WriteJob(w, user, job.ID)
 }
 
-func (p *MyPlugin) GetJob(w launcher.ResponseWriter, user string, id api.JobID, fields []string) {
-    p.cache.WriteJob(w, user, string(id))
+func (p *MyPlugin) GetJob(_ context.Context, w launcher.ResponseWriter, user string, id api.JobID, fields []string) {
+    p.cache.WriteJob(w, user, id)
 }
 
 // ... implement other Plugin methods ...
@@ -111,16 +111,16 @@ All plugins implement the `launcher.Plugin` interface:
 
 ```go
 type Plugin interface {
-    SubmitJob(w ResponseWriter, user string, job *api.Job)
-    GetJob(w ResponseWriter, user string, id api.JobID, fields []string)
-    GetJobs(w ResponseWriter, user string, filter *api.JobFilter, fields []string)
-    ControlJob(w ResponseWriter, user string, id api.JobID, op api.JobOperation)
+    SubmitJob(ctx context.Context, w ResponseWriter, user string, job *api.Job)
+    GetJob(ctx context.Context, w ResponseWriter, user string, id api.JobID, fields []string)
+    GetJobs(ctx context.Context, w ResponseWriter, user string, filter *api.JobFilter, fields []string)
+    ControlJob(ctx context.Context, w ResponseWriter, user string, id api.JobID, op api.JobOperation)
     GetJobStatus(ctx context.Context, w StreamResponseWriter, user string, id api.JobID)
     GetJobStatuses(ctx context.Context, w StreamResponseWriter, user string)
     GetJobOutput(ctx context.Context, w StreamResponseWriter, user string, id api.JobID, outputType api.JobOutput)
     GetJobResourceUtil(ctx context.Context, w StreamResponseWriter, user string, id api.JobID)
-    GetJobNetwork(w ResponseWriter, user string, id api.JobID)
-    ClusterInfo(w ResponseWriter, user string)
+    GetJobNetwork(ctx context.Context, w ResponseWriter, user string, id api.JobID)
+    ClusterInfo(ctx context.Context, w ResponseWriter, user string)
 }
 ```
 
@@ -147,7 +147,12 @@ cache.StreamJobStatus(ctx, w, user, jobID)
 Write tests using the provided mocks and builders:
 
 ```go
-import "github.com/posit-dev/launcher-go-sdk/plugintest"
+import (
+    "context"
+    "testing"
+
+    "github.com/posit-dev/launcher-go-sdk/plugintest"
+)
 
 func TestSubmitJob(t *testing.T) {
     w := plugintest.NewMockResponseWriter()
@@ -158,7 +163,7 @@ func TestSubmitJob(t *testing.T) {
         WithCommand("echo hello").
         Build()
 
-    plugin.SubmitJob(w, "alice", job)
+    plugin.SubmitJob(context.Background(), w, "alice", job)
 
     plugintest.AssertNoError(t, w)
     plugintest.AssertJobCount(t, w, 1)
@@ -189,7 +194,7 @@ Once we release v1.0.0, we will strictly maintain backwards compatibility within
 
 ## Requirements
 
-- Go 1.25 or later
+- Go 1.24 or later
 - Linux, macOS (we test on Linux and macOS)
 - Workbench 2023.09.0 or later or Connect 2024.08.0 or later (for deployment)
 

api/types.go

@@ -110,7 +110,11 @@ func (e *Error) Error() string {
 	return e.Code.String()
 }
 
-// Is makes comparisons with errors.Is() possible.
+// Is reports whether err matches e by error code. When either error has an
+// empty message, only the code is compared. This enables sentinel-style usage
+// where a code-only Error (e.g., ErrJobNotRunning) matches any Error with the
+// same code regardless of its message text. When both errors have non-empty
+// messages, they must also match for Is to return true.
 func (e *Error) Is(err error) bool {
 	apiErr, ok := err.(*Error)
 	if !ok {
@@ -271,10 +275,16 @@ func (o *JobOutput) UnmarshalText(text []byte) error {
 	return nil
 }
 
-// JobID is represented by a string, but may be "*" in some cases to indicate
-// any or all jobs.
+// JobID is a string-based job identifier. The special value "*" (see
+// [JobIDWildcard]) indicates any or all jobs in some API contexts.
 type JobID string
 
+// JobIDWildcard is the sentinel value meaning "any or all jobs."
+const JobIDWildcard JobID = "*"
+
+// IsWildcard reports whether the JobID is the wildcard value.
+func (id JobID) IsWildcard() bool { return id == JobIDWildcard }
+
 // Container holds container fields for a Job.
 type Container struct {
 	// The name of the container image to use.
@@ -326,7 +336,7 @@ func (e *Env) Set(s string) error {
 // Job is Launcher's representation of a job.
 type Job struct {
 	// The unique ID of the Job.
-	ID string `json:"id"`
+	ID JobID `json:"id"`
 
 	// The cluster of the Job. Optional.
 	Cluster string `json:"cluster,omitempty"`

api/types_test.go

@@ -180,6 +180,16 @@ func TestJobID(t *testing.T) {
 	if string(id) != "test-123" {
 		t.Errorf("JobID conversion failed: got %q, want %q", string(id), "test-123")
 	}
+
+	if id.IsWildcard() {
+		t.Error("regular JobID should not be wildcard")
+	}
+	if !JobIDWildcard.IsWildcard() {
+		t.Error("JobIDWildcard should be wildcard")
+	}
+	if string(JobIDWildcard) != "*" {
+		t.Errorf("JobIDWildcard = %q, want %q", string(JobIDWildcard), "*")
+	}
 }
 
 func TestJobOutput(t *testing.T) {