Fix /ctx error handling in interactive REPL#47
Closed
superbling wants to merge 1 commit intoantirez:mainfrom
Closed
Fix /ctx error handling in interactive REPL#47superbling wants to merge 1 commit intoantirez:mainfrom
superbling wants to merge 1 commit intoantirez:mainfrom
Conversation
In REPL, parse_int() exits the process via exit(2) on invalid input, which is correct for command-line parsing but kills the interactive session, dropping the KV cache and conversation. Add parse_int_repl() which prints the same error and returns false instead, so the REPL can just prompt again. repl_chat_set_ctx() freed the existing session before trying to create the replacement. If creation failed (e.g. allocation error for the new context size), the old session was already gone and run_repl broke out of the loop. Drop the redundant pre-free; repl_chat_create_session already keeps the old session intact on failure, so /ctx can now report the error and keep the current context.
Author
|
Closing in favor of two smaller PRs that split this work apart:
This original PR bundled both, but the second piece is really a design call about whether Sorry for the noise. |
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
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
Fix two issues with the
/ctxcommand that caused the interactive REPL to terminate unexpectedly./ctxwith an invalid value kills the process. The REPL handler calledparse_int(), which callsexit(2)on bad input. That is correct for command-line argument parsing, but in the REPL it drops the KV cache and the entire conversation on a typo (e.g./ctx abc). Added aparse_int_repl()twin that prints the same error and returnsfalse, so the REPL can simply prompt again./ctxwith a valid value but failing session creation also kills the chat.repl_chat_set_ctx()freed the existing session before trying to create the replacement. If creation failed (e.g. allocation error for the new context size), the old session was already gone andrun_replbroke out of the loop. Dropped the redundant pre-free;repl_chat_create_session()already keeps the old session intact on failure, so/ctxnow reports the error and keeps the current context.Test plan
make— clean build, no new warnings./ds4 -c 4096:/ctx abc→ printsds4: invalid value for /ctx: abc, REPL continues (previously: process exit)/ctx 4096→ successfully recreates the session (logs new context buffers)/quit→ exits cleanly