Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 136
feat: implement state persistence#177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
e3bd936
chore(lib): extract Conversation interface
johnstcn e5f1bda
Merge branch 'main' into cj/refactor-conversation
35C4n0r a0f8bb5
feat: implement state persistence
35C4n0r ca3cdff
feat: pid file writing and clearing and improved error handling for l…
35C4n0r 1c224e9
refactor: remove redundant save logic
35C4n0r 30f82d7
feat: improve logic for first run with empty state file
35C4n0r 12bed1c
feat: implement platform-specific signal handling
35C4n0r e366e8b
feat: refactor cfg -> Config and move pid ops to server
35C4n0r 26fdf81
feat: unregister the signal handlers on teardown
35C4n0r 021e33f
Merge branch 'main' into 35C4n0r/agentapi-state-persistence
35C4n0r 5795db7
feat: resolve conflicts and improve shutdown sequence
35C4n0r b44fe5d
Merge branch 'main' into 35C4n0r/agentapi-state-persistence
35C4n0r 9deab88
feat: resolve conflicts
35C4n0r 18fb1e4
chore: not dirty after load state
35C4n0r b719dac
feat: add tests
35C4n0r 3959002
feat: remove comment
35C4n0r 7e389d2
feat: remove comments
35C4n0r 1d7aaed
wip: address comments
35C4n0r 058b18f
feat: remove anti-pattern for graceful shutdown
35C4n0r 2565a3c
feat: remove additional message upon load state fail
35C4n0r 1033cd7
wip: apply suggestions from cian
35C4n0r cfb7601
wip: apply suggestions from cian
35C4n0r 9d7eb5a
feat: update tests
35C4n0r 759ec53
feat: improved initial prompt handling
35C4n0r 03c6f16
chore: comments
35C4n0r bd75240
chore: address cian's file permission comments
35C4n0r b1ab615
feat: implement error handling for agent events
35C4n0r 31d27a7
fix: no screen adjustment in case of loadState failure
35C4n0r 220d360
feat: add three e2e tests for statePersistence
35C4n0r eef927d
feat: address maf's review
35C4n0r 33460d2
feat: address ai's review
35C4n0r ad19496
feat: address maf's comments and remove adjustScreenAfterLoadState
35C4n0r 7c42d35
chore: add missing files
35C4n0r d7d7744
feat: add check for existing pid
35C4n0r b2cbf56
fix: address review findings from #177 (#195)
mafredri 410e29b
Merge branch 'main' into 35C4n0r/agentapi-state-persistence
35C4n0r db97306
feat: check for conflicting ACP and state persistence flags
35C4n0r 2fd2110
feat: fix tests
35C4n0r f1b6ba6
chore: throw error on file not found
35C4n0r 2188089
chore: don't emit file not found error
35C4n0r File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| //go:build unix | ||
| package server | ||
| import ( | ||
| "errors" | ||
| "os" | ||
| "syscall" | ||
| ) | ||
| // isProcessRunning checks if a process with the given PID is running. | ||
| func isProcessRunning(pid int) bool { | ||
| process, err := os.FindProcess(pid) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| err = process.Signal(syscall.Signal(0)) | ||
| return err == nil || errors.Is(err, syscall.EPERM) | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| //go:build windows | ||
| package server | ||
| // isProcessRunning checks if a process with the given PID is running. | ||
| // On Windows, Signal(0) is not supported, so this always returns false. | ||
| // PID file liveness detection is best-effort on this platform. | ||
| func isProcessRunning(_ int) bool { | ||
| return false | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -8,9 +8,13 @@ import ( | ||
| "log/slog" | ||
| "net/http" | ||
| "os" | ||
| "path/filepath" | ||
| "sort" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
| "github.com/coder/agentapi/lib/screentracker" | ||
| "github.com/mattn/go-isatty" | ||
| "github.com/spf13/cobra" | ||
| "github.com/spf13/viper" | ||
| @@ -104,9 +108,51 @@ func runServer(ctx context.Context, logger *slog.Logger, argsToPass []string) er | ||
| } | ||
| } | ||
| printOpenAPI := viper.GetBool(FlagPrintOpenAPI) | ||
| // Get the variables related to state management | ||
| stateFile := viper.GetString(FlagStateFile) | ||
| loadState := false | ||
| saveState := false | ||
| // Validate state file configuration | ||
| if stateFile != "" { | ||
| if !viper.IsSet(FlagLoadState) { | ||
| loadState = true | ||
| } else { | ||
| loadState = viper.GetBool(FlagLoadState) | ||
| } | ||
| if !viper.IsSet(FlagSaveState) { | ||
| saveState = true | ||
| } else { | ||
| saveState = viper.GetBool(FlagSaveState) | ||
| } | ||
| } else { | ||
| if viper.IsSet(FlagLoadState) && viper.GetBool(FlagLoadState) { | ||
| return xerrors.Errorf("--load-state requires --state-file to be set") | ||
| } | ||
| if viper.IsSet(FlagSaveState) && viper.GetBool(FlagSaveState) { | ||
| return xerrors.Errorf("--save-state requires --state-file to be set") | ||
| } | ||
| } | ||
| experimentalACP := viper.GetBool(FlagExperimentalACP) | ||
| if experimentalACP && (saveState || loadState) { | ||
| return xerrors.Errorf("ACP mode doesn't support state persistence") | ||
| } | ||
| pidFile := viper.GetString(FlagPidFile) | ||
| // Write PID file if configured | ||
| if pidFile != "" { | ||
| if err := writePIDFile(pidFile, logger); err != nil { | ||
| return xerrors.Errorf("failed to write PID file: %w", err) | ||
| } | ||
| defer cleanupPIDFile(pidFile, logger) | ||
| } | ||
| printOpenAPI := viper.GetBool(FlagPrintOpenAPI) | ||
| if printOpenAPI && experimentalACP { | ||
| return xerrors.Errorf("flags --%s and --%s are mutually exclusive", FlagPrintOpenAPI, FlagExperimentalACP) | ||
| } | ||
| @@ -154,33 +200,45 @@ func runServer(ctx context.Context, logger *slog.Logger, argsToPass []string) er | ||
| AllowedHosts: viper.GetStringSlice(FlagAllowedHosts), | ||
| AllowedOrigins: viper.GetStringSlice(FlagAllowedOrigins), | ||
| InitialPrompt: initialPrompt, | ||
| StatePersistenceConfig: screentracker.StatePersistenceConfig{ | ||
| StateFile: stateFile, | ||
| LoadState: loadState, | ||
| SaveState: saveState, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return xerrors.Errorf("failed to create server: %w", err) | ||
| } | ||
| if printOpenAPI { | ||
| fmt.Println(srv.GetOpenAPI()) | ||
| return nil | ||
| } | ||
| // Create a context for graceful shutdown | ||
| gracefulCtx, gracefulCancel := context.WithCancel(ctx) | ||
| defer gracefulCancel() | ||
| // Setup signal handlers (they will call gracefulCancel) | ||
| handleSignals(gracefulCtx, gracefulCancel, logger, srv) | ||
| logger.Info("Starting server on port", "port", port) | ||
| // Monitor process exit | ||
| processExitCh := make(chan error, 1) | ||
| // Wait for process exit in PTY mode | ||
| if process != nil { | ||
| go func() { | ||
| defer close(processExitCh) | ||
| defer gracefulCancel() | ||
| if err := process.Wait(); err != nil { | ||
| if errors.Is(err, termexec.ErrNonZeroExitCode) { | ||
| processExitCh <- xerrors.Errorf("========\n%s\n========\n: %w", strings.TrimSpace(process.ReadScreen()), err) | ||
| } else { | ||
| processExitCh <- xerrors.Errorf("failed to wait for process: %w", err) | ||
| } | ||
| } | ||
| if err := srv.Stop(ctx); err != nil { | ||
| logger.Error("Failed to stop server", "error", err) | ||
| } | ||
| }() | ||
| } | ||
| // Wait for process exit in ACP mode | ||
| if acpResult != nil { | ||
| go func() { | ||
| defer close(processExitCh) | ||
| @@ -193,13 +251,45 @@ func runServer(ctx context.Context, logger *slog.Logger, argsToPass []string) er | ||
| } | ||
| }() | ||
| } | ||
| if err := srv.Start(); err != nil && err != context.Canceled && err != http.ErrServerClosed { | ||
| return xerrors.Errorf("failed to start server: %w", err) | ||
| // Start the server | ||
| serverErrCh := make(chan error, 1) | ||
| go func() { | ||
| defer close(serverErrCh) | ||
| if err := srv.Start(); err != nil && !errors.Is(err, context.Canceled) && !errors.Is(err, http.ErrServerClosed) { | ||
| serverErrCh <- err | ||
| } | ||
| }() | ||
| select { | ||
| case err := <-serverErrCh: | ||
| if err != nil { | ||
| return xerrors.Errorf("failed to start server: %w", err) | ||
| } | ||
| case <-gracefulCtx.Done(): | ||
| } | ||
| if err := srv.SaveState("shutdown"); err != nil { | ||
| logger.Error("Failed to save state during shutdown", "error", err) | ||
| } | ||
| // Stop the HTTP server | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| if err := srv.Stop(shutdownCtx); err != nil { | ||
| logger.Error("Failed to stop HTTP server", "error", err) | ||
| } | ||
| select { | ||
| case err := <-processExitCh: | ||
| return xerrors.Errorf("agent exited with error: %w", err) | ||
| if err != nil { | ||
| return xerrors.Errorf("agent exited with error: %w", err) | ||
| } | ||
| default: | ||
| // Close the process | ||
| if err := process.Close(logger, 5*time.Second); err != nil { | ||
| logger.Error("Failed to close process cleanly", "error", err) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| @@ -213,6 +303,61 @@ var agentNames = (func() []string { | ||
| return names | ||
| })() | ||
| // writePIDFile writes the current process ID to the specified file | ||
| func writePIDFile(pidFile string, logger *slog.Logger) error { | ||
| pid := os.Getpid() | ||
| pidContent := fmt.Sprintf("%d\n", pid) | ||
| // Create directory if it doesn't exist | ||
| dir := filepath.Dir(pidFile) | ||
| if err := os.MkdirAll(dir, 0o700); err != nil { | ||
| return xerrors.Errorf("failed to create PID file directory: %w", err) | ||
| } | ||
| // Check if PID file already exists | ||
| if existingPIDData, err := os.ReadFile(pidFile); err == nil { | ||
| existingPIDStr := strings.TrimSpace(string(existingPIDData)) | ||
| if existingPID, err := strconv.Atoi(existingPIDStr); err == nil { | ||
| if isProcessRunning(existingPID) { | ||
| return xerrors.Errorf("another instance is already running with PID %d (PID file: %s)", existingPID, pidFile) | ||
| } | ||
| logger.Warn("Found stale PID file, will overwrite", "pidFile", pidFile, "stalePID", existingPID) | ||
| } | ||
| } else if !os.IsNotExist(err) { | ||
| return xerrors.Errorf("failed to read existing PID file: %w", err) | ||
| } | ||
| // Write PID file | ||
| if err := os.WriteFile(pidFile, []byte(pidContent), 0o600); err != nil { | ||
| return xerrors.Errorf("failed to write PID file: %w", err) | ||
| } | ||
| logger.Info("Wrote PID file", "pidFile", pidFile, "pid", pid) | ||
| return nil | ||
| } | ||
35C4n0r marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // cleanupPIDFile removes the PID file if it was written by this process. | ||
| func cleanupPIDFile(pidFile string, logger *slog.Logger) { | ||
| data, err := os.ReadFile(pidFile) | ||
| if err != nil { | ||
| if !os.IsNotExist(err) { | ||
| logger.Error("Failed to read PID file for cleanup", "pidFile", pidFile, "error", err) | ||
| } | ||
| return | ||
| } | ||
| pidStr := strings.TrimSpace(string(data)) | ||
| filePID, err := strconv.Atoi(pidStr) | ||
| if err != nil || filePID != os.Getpid() { | ||
| logger.Info("PID file belongs to another process, skipping cleanup", "pidFile", pidFile, "filePID", pidStr) | ||
| return | ||
| } | ||
| if err := os.Remove(pidFile); err != nil && !os.IsNotExist(err) { | ||
| logger.Error("Failed to remove PID file", "pidFile", pidFile, "error", err) | ||
| } else if err == nil { | ||
| logger.Info("Removed PID file", "pidFile", pidFile) | ||
| } | ||
| } | ||
| type flagSpec struct { | ||
| name string | ||
| shorthand string | ||
| @@ -232,6 +377,10 @@ const ( | ||
| FlagAllowedOrigins = "allowed-origins" | ||
| FlagExit = "exit" | ||
| FlagInitialPrompt = "initial-prompt" | ||
| FlagStateFile = "state-file" | ||
| FlagLoadState = "load-state" | ||
| FlagSaveState = "save-state" | ||
| FlagPidFile = "pid-file" | ||
| FlagExperimentalACP = "experimental-acp" | ||
| ) | ||
| @@ -271,6 +420,10 @@ func CreateServerCmd() *cobra.Command { | ||
| // localhost:3284 is the default origin when you open the chat interface in your browser. localhost:3000 and 3001 are used during development. | ||
| {FlagAllowedOrigins, "o", []string{"http://localhost:3284", "http://localhost:3000", "http://localhost:3001"}, "HTTP allowed origins. Use '*' for all, comma-separated list via flag, space-separated list via AGENTAPI_ALLOWED_ORIGINS env var", "stringSlice"}, | ||
| {FlagInitialPrompt, "I", "", "Initial prompt for the agent. Recommended only if the agent doesn't support initial prompt in interaction mode. Will be read from stdin if piped (e.g., echo 'prompt' | agentapi server -- my-agent)", "string"}, | ||
| {FlagStateFile, "s", "", "Path to file for saving/loading server state", "string"}, | ||
| {FlagLoadState, "", false, "Load state from state-file on startup (defaults to true when state-file is set)", "bool"}, | ||
| {FlagSaveState, "", false, "Save state to state-file on shutdown (defaults to true when state-file is set)", "bool"}, | ||
| {FlagPidFile, "", "", "Path to file where the server process ID will be written for shutdown scripts", "string"}, | ||
| {FlagExperimentalACP, "", false, "Use experimental ACP transport instead of PTY", "bool"}, | ||
| } | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider moving this into
case <-gracefulCtx.Done():above? I'm guessing we don't have to callsrv.Stopif we receive onserverErrCh.Does stop error if the server already closed? If yes, we'll end up printing a misleading error here.
EDIT: I looked at Stop and the once does guard against multiple Stops, but not against Stop producing an error if the server closed before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We return early if we receive anything(non nil) on serverErrCh
True, I had an alternative in mind, but I decided to proceed with
oncehere for the above-mentioned reason. I'll add a check inStopI'll check for this ErrServerClosed, and return nil in that case.