docs: add ARCHITECTURE.md with mermaid diagrams#4
Conversation
Three diagrams covering the as-built system (through PR #3, Phase 2 step 0): 1. Layered architecture — network -> connection -> protocol -> command -> store, with planned Phase 2.6-4 additions shown in dashed yellow 2. Per-command sequence — autonumbered walk through one SET k v from raw TCP bytes to "+OK\r\n", showing the Connection task, parser, dispatch, tokio::sync::Mutex acquire, handler, and serialize back 3. State + ownership — splits state into three lifetimes (process-wide, per-connection, per-command) so the lock semantics are unambiguous Also includes a file-map table mapping each layer to its source files. Renders natively in the GitHub markdown preview. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds ChangesArchitecture Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7ce84f493
ℹ️ 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".
| CO->>D: dispatch(cmd, &store).await | ||
| D->>M: lock().await |
There was a problem hiding this comment.
This sequence documents a non-existent async dispatch/tokio-mutex path: dispatch is a synchronous function (redis-clone/src/command/dispatch.rs:22) and SharedStore is Arc<std::sync::Mutex<Store>> (redis-clone/src/store/mod.rs:2-6), with handlers calling store.lock().unwrap() themselves. Because this doc is presented as the as-built architecture, the current wording puts guard ownership in the wrong layer and will mislead anyone implementing transactions or auditing lock scope from it.
Useful? React with 👍 / 👎.
| ```mermaid | ||
| flowchart TB | ||
| subgraph Process["Process-wide (one per server)"] | ||
| NEXT["static NEXT_CLIENT_ID: AtomicU64"] |
There was a problem hiding this comment.
This state diagram shows a process-wide NEXT_CLIENT_ID and per-connection IDs as existing state, but the current code has no such static and handle_connection only receives (TcpStream, SharedStore) (redis-clone/src/server/connection.rs:10). Since the document says it describes what has actually been built, this makes connection identity look available to future CLIENT/WATCH work when it is not implemented yet.
Useful? React with 👍 / 👎.
|
|
||
| | Layer | Files | | ||
| |---|---| | ||
| | Network | `src/server/listener.rs` | |
There was a problem hiding this comment.
The file map starts with root-relative src/... paths, but the repository has no root src directory; the actual files are under redis-clone/src/... (for example redis-clone/src/server/listener.rs). As written, the navigation table sends readers and tooling to paths that do not exist in this repo.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ARCHITECTURE.md`:
- Line 169: The diagram's arrow "guard --> Process" is misleading about
ownership; update the diagram to either remove that arrow or replace it with a
clarifying annotation such as "guard dropped → lock released" that explains that
MutexGuard (the per-command guard) is dropped at end of the command lifetime
which releases the lock on the process-wide Mutex<Store>; make the change where
the "guard" and "Process" nodes are defined so readers see the correct lifecycle
relationship for MutexGuard and Mutex<Store>.
- Around line 185-194: Update the table so every filepath entry is prefixed with
"redis-clone/" (e.g., change "src/server/listener.rs" to
"redis-clone/src/server/listener.rs") for all layers (Network, Per-conn,
Protocol, Command, Store, Errors, Entry, Tests) and also add the missing module
files by including "redis-clone/src/server/mod.rs",
"redis-clone/src/protocol/mod.rs", and "redis-clone/src/command/mod.rs"
alongside the existing specific files (e.g., keep
"redis-clone/src/server/listener.rs", "redis-clone/src/server/connection.rs"
which contains the Connection struct and handle_connection wrapper, and the
protocol/command files listed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| NEXT -- fetch_add(1) --> cid | ||
| STORE_ARC -- "Arc::clone()" --> sharehandle | ||
| sharehandle -- "lock().await" --> guard | ||
| guard --> Process |
There was a problem hiding this comment.
Clarify or remove the guard --> Process arrow.
The arrow from guard back to Process is semantically unclear. The MutexGuard doesn't "point to" the Process scope; it's dropped at the end of the per-command lifetime, which releases the lock on the process-wide Mutex<Store>. This arrow might confuse readers about ownership semantics.
Consider removing this arrow or replacing it with a note like "guard dropped → lock released" to clarify the relationship.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ARCHITECTURE.md` at line 169, The diagram's arrow "guard --> Process" is
misleading about ownership; update the diagram to either remove that arrow or
replace it with a clarifying annotation such as "guard dropped → lock released"
that explains that MutexGuard (the per-command guard) is dropped at end of the
command lifetime which releases the lock on the process-wide Mutex<Store>; make
the change where the "guard" and "Process" nodes are defined so readers see the
correct lifecycle relationship for MutexGuard and Mutex<Store>.
| | Layer | Files | | ||
| |---|---| | ||
| | Network | `src/server/listener.rs` | | ||
| | Per-conn | `src/server/connection.rs` (`Connection` struct + `handle_connection` wrapper) | | ||
| | Protocol | `src/protocol/{types,parser,serializer}.rs` | | ||
| | Command | `src/command/{types,dispatch}.rs` + `src/command/handlers/{ping,echo,set,get,del,exists,list,hash,set_cmd}.rs` | | ||
| | Store | `src/store/mod.rs` | | ||
| | Errors | `src/error.rs` | | ||
| | Entry | `src/main.rs`, `src/lib.rs` | | ||
| | Tests | `tests/phase1.rs` (integration), inline `#[cfg(test)]` modules in each handler file | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check existence of files mentioned in the file map
echo "=== Checking individual files ==="
for file in \
"src/server/listener.rs" \
"src/server/connection.rs" \
"src/protocol/types.rs" \
"src/protocol/parser.rs" \
"src/protocol/serializer.rs" \
"src/command/types.rs" \
"src/command/dispatch.rs" \
"src/store/mod.rs" \
"src/error.rs" \
"src/main.rs" \
"src/lib.rs" \
"tests/phase1.rs"
do
if [ -f "$file" ]; then
echo "✓ $file"
else
echo "✗ MISSING: $file"
fi
done
echo -e "\n=== Checking handler files directory ==="
if [ -d "src/command/handlers" ]; then
echo "✓ src/command/handlers/ directory exists"
echo "Handler files found:"
fd -e rs . src/command/handlers/
else
echo "✗ MISSING: src/command/handlers/ directory"
fiRepository: agaonker/redis-rust
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Discover actual repository structure
echo "=== Top-level files and directories ==="
ls -la | head -20
echo -e "\n=== src/ directory structure (if exists) ==="
if [ -d "src" ]; then
find src -type f -name "*.rs" | head -20
echo "---"
find src -type d | sort
else
echo "src/ directory does not exist"
fi
echo -e "\n=== tests/ directory structure (if exists) ==="
if [ -d "tests" ]; then
find tests -type f -name "*.rs" | head -20
else
echo "tests/ directory does not exist"
fi
echo -e "\n=== Total Rust files in repository ==="
find . -name "*.rs" -type f | wc -l
echo "Rust files found:"
find . -name "*.rs" -type fRepository: agaonker/redis-rust
Length of output: 1733
Update file paths in the file map table to include the redis-clone/ directory prefix.
All files referenced in the table exist, but each path is missing the redis-clone/ directory prefix. Paths should be relative to the repository root and must start with redis-clone/ (e.g., redis-clone/src/server/listener.rs instead of src/server/listener.rs). This affects all entries in the table: Network, Per-conn, Protocol, Command, Store, Errors, Entry, and Tests layers. Additionally, the table omits the module files (src/server/mod.rs, src/protocol/mod.rs, src/command/mod.rs) which do exist in the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ARCHITECTURE.md` around lines 185 - 194, Update the table so every filepath
entry is prefixed with "redis-clone/" (e.g., change "src/server/listener.rs" to
"redis-clone/src/server/listener.rs") for all layers (Network, Per-conn,
Protocol, Command, Store, Errors, Entry, Tests) and also add the missing module
files by including "redis-clone/src/server/mod.rs",
"redis-clone/src/protocol/mod.rs", and "redis-clone/src/command/mod.rs"
alongside the existing specific files (e.g., keep
"redis-clone/src/server/listener.rs", "redis-clone/src/server/connection.rs"
which contains the Connection struct and handle_connection wrapper, and the
protocol/command files listed).
1 participant
Summary
ARCHITECTURE.mdat the repo root with three mermaid diagrams covering the as-built system through PR Phase 2 step 0: Connection struct + tokio::sync::Mutex #3 (Phase 2 step 0).What's in it
SET k vfrom raw TCP bytes to+OK\r\n, showing the Connection task, parser, dispatch, tokio mutex acquire, handler, MutexGuard release, and serialize back.Plus a file-map table at the bottom mapping each layer to its source files.
Test plan
<,>,&inside labelsredis-rust-design.mdandTODOS.mdare correct🤖 Generated with Claude Code
Summary by CodeRabbit