feat(shell): redis console store with cross-language streaming integ - #795
Merged
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c95061fb35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Split job_table into types/constants/table in both languages, mirroring the console package. Redis console review fixes: the minted key prefix is public (JobConsole .store plus RedisConsoleStore.key_prefix) so an external reader can be handed a console's address; keys expire ttl_seconds after the last append (default one day) instead of accumulating; the ending chunk is terminal in the store itself, so an emit racing a kill past the local guard is dropped server-side; and the TS loader validates the console block's value types the way Pydantic already did.
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.
Follow-up to #611. The
ConsoleStorecontract was shaped like a stream on purpose; this lands the second implementation to prove it, so a job's console can be read from another process (or another language) while the job runs, and makes the console backend a config-level switch.RedisConsoleStore(both languages)mirage/shell/console/redis/and@struktoai/mirage-nodeshell/console/redis/, one Redis stream per job. Each package holdsstore+constants+append.lua, following thecas.luaprecedent: the Lua ships beside its module (importlib.resourcesin Python;readFileSync(new URL(...))plus a tsup dist copy in TypeScript), and the two copies are pinned byte-identical by a test.seqmaps to stream id(seq+1)-0; channel, payload and timestamp ride as entry fields (c/d/t).append.lua(INCR+ explicit-idXADD) hands out dense seqs atomically, so a kill marker racing a runner's last emit cannot collide on an id. Both languages write byte-identical schema, and each side's unit tests assert the same pinned raw-wire shape, so a reader in the other language attaches to a stream this one wrote by construction.waitparks server-side (XREAD BLOCK) in short rounds so a localclose()is noticed within one round. TypeScript polls the seq counter instead: node-redis serializes commands on one connection, so a server-side block would wedge the job's own appends behind it. Documented in place, mirroring the RAM store's own py/ts asymmetry note.read_fromnever reports a truncated cursor; the contract's truncation channel stays for stores that do trim.The backend is selected at runtime, not compiled in
JobTabletakes aconsole_factory/consoleFactory(job id ->JobConsole), threaded throughWorkspaceas a constructor knob beside the other store overrides.console:block in the workspace YAML ({type: ram}default,{type: redis, url, key_prefix}), a discriminated block exactly likecache:/index:/store:, validated by both loaders and covered by the shared accept/reject fixtures (integ/fixtures/config/*.json). The config-built factory mints a fresh nonce beside each job id, because ids restart at 1 when the table empties and a reused stream would replay the previous job's chunks.redispackage stays a lazily-loaded optional peer.Retention, addressing and the terminal chunk
Three properties the store enforces itself, all inside
append.luaso they are atomic with the write:RedisConsoleStore.key_prefix/keyPrefix, reachable asjob.console.store. That is what an embedder hands to the process that should attach.ttl_seconds(default one day,nullto opt out) issues anEXPIREon every append, so a console expires that long after its job's last write whether or not the job ended cleanly.endedkey with the CONTROL chunk and refuses later appends, so an emit that raced a kill pastJobConsole's local guard is dropped server-side rather than landing past the ending where no follower would read it. That matches the RAM store andemit's documented after-the-ending semantics.Ownership
The job table tracks what the factory builds and the workspace closes it at teardown (
close_consoles/closeConsoles, afterkill_all): a config-provisioned store keeps a client open per job, invisible to the embedder, and in Node an open client holds the process alive. A console still outlives its table entry, so a reap never closes one, and default in-memory consoles are never tracked.The
_settleshieldThe kill path in
_settleemits while the task is already unwinding from a cancel. RAM never suspends there; Redis does, and a second cancellation mid-write would lose theKilledmarker readers are parked on. The marker + ending write is now wrapped inasyncio.shield. The regression test was verified both ways: it times out on the unshielded code and passes with the shield.Battery coverage instead of bespoke scripts
A new
console-redistarget (ram mounts, consoles on Redis via the target'sconsoleblock) rides the shared battery besideram:integ/console/jobs.jsonruns the same four job-control cases (& waitoutput adoption,wait $!exit codes, kill by pid, background writes landing) on both hosts x both backends from one JSON file. The runners gained theconsolecase dir, the target key, and a refusal for a console block on any non-ram resource, so a silently RAM-consoled "redis" target cannot read as covered.job_tableis a package nowshell/job_table.pyandshell/job_table.tssplit intotypes(Job, JobStatus, JobRunner, ConsoleFactory),constants(KILLED_EXIT_CODE) andtable(JobTable, cancel_job/settle), mirroring the console package's layout in both languages. Import paths are unchanged for every caller; the tests moved to match (tests/shell/job_table/test_table.py,shell/job_table/table.test.ts). Layout parity holds at its committed baseline.Tests
tests/shell/console/redis/and nodeshell/console/redis/store.test.ts(skipped withoutREDIS_URL): dense seqs, cursor+limit reads, wake-on-append, close releases a parked waiter, a follow across two store instances sharing only the key prefix, the pinned raw wire schema, drop-after-ending, TTL applied and absent, and the byte-identity of the twoappend.luacopies.test_job_table.py/job_table.test.ts:close_consolesreleases factory stores and leaves default consoles alone; the second-cancel shield test.test_loader.pyand serverconfig.test.tscover the redis form (factory with fresh keys) and the ram form (no factory), plus the shared accept/reject fixtures (which now pin a non-stringurland a zerottl_secondsas rejected by both loaders).