Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAsync bcrypt ops now accept a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Awaiting Coroutine
participant Op as bcrypt async op
participant Env as capy::io_env
participant Ex as Executor
Caller->>Op: co_await op (await_suspend(cont, env))
Op->>Env: store env pointer
Op->>Ex: auto caller_ex = env->executor
Op->>Ex: register completion callback (lambda)
Note over Ex,Op: async work proceeds...
Ex-->>Op: invoke completion callback
Op->>Ex: caller_ex.dispatch(cont).resume()
Ex->>Caller: resume coroutine
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
|
An automated preview of the documentation is available at https://267.http.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-21 13:26:57 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/server/serve_static.cpp (1)
10-16: Missing high-level implementation overview comment.
serve_static.cppcontains non-trivial logic (dotfile policy, range handling, index fallback, serializer buffer protocol) but has no/* */block comment after the#includesection explaining how the implementation works.As per coding guidelines: "Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works. This should highlight anything tricky or non-obvious that a maintainer might need to know in order to reason effectively about the code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/serve_static.cpp` around lines 10 - 16, Add a /* ... */ block comment immediately after the `#include` section in serve_static.cpp that gives a concise high-level overview of the implementation: describe the dotfile policy enforcement, how HTTP range requests are parsed/handled (including partial vs full responses), the index-file fallback behavior for directory requests, and the serializer buffer protocol used when streaming files (buffer sizes, lifetime/ownership expectations). Reference the top-level symbols serve_static (or the module-level functions/types in this file) and any helpers that implement range parsing/streaming so readers know which pieces of code the overview applies to.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/http/bcrypt.hpp`:
- Around line 533-553: The await_suspend method currently dereferences env
(env->executor) without validation, which can crash if the awaitable is used
outside a capy io_env; in await_suspend (the lambda using caller_ex and the
calls to caller_ex.dispatch(cont).resume()) add a null-check for env (or
env->executor) and handle the error by capturing/setting ep_ to a descriptive
std::runtime_error (or call cont.resume on the caller with an exception) so
misuse fails fast and clearly, or alternatively document in the header that
await_suspend (and the hash_async/compare_async awaitables that call
hash_task/compare_task) require being co_awaited inside a capy io_env. Ensure
the check references await_suspend, env->executor, and the
hash_task/compare_task call sites so reviewers can locate the change.
In `@src/server/serve_static.cpp`:
- Around line 317-324: The loop currently breaks on a file read error
(ec.failed()) and later calls commit_eof(0), silently sending a truncated body;
instead detect the error from f.read() and propagate it immediately: replace the
break-on-error with co_return route_error(ec) (or an equivalent error return) so
read failures from f.read(...) are logged/returned and no partial response is
committed; keep the existing EOF handling for n1 == 0 unchanged. Ensure you
modify the block around f.read, rp.res_body.commit, and the subsequent
commit_eof(0) logic (referencing f.read, ec.failed(), rp.res_body.commit,
route_error, commit_eof) so that only EOF continues to commit_eof and any read
error returns route_error.
- Around line 303-311: The loop that calls rp.res_body.prepare(arr) and, on
empty result, awaits rp.res_body.commit(0) lacks a forward-progress cap; update
serve_static.cpp to either document the guarantee from capy that a successful
co_await rp.res_body.commit(0) will make space (i.e., commit triggers underlying
write_some and frees buffer) or implement a bounded retry/timeout: add a max
retry counter (e.g., MAX_PREPARE_RETRIES) around the prepare/commit loop, after
which return route_error or a timeout error, and optionally insert a short async
backoff/yield between commits; ensure you reference and modify the loop that
uses capy::mutable_buffer arr[], rp.res_body.prepare(...), and
rp.res_body.commit(0) and return route_error(ec) when the retry limit or timeout
is reached.
---
Nitpick comments:
In `@src/server/serve_static.cpp`:
- Around line 10-16: Add a /* ... */ block comment immediately after the
`#include` section in serve_static.cpp that gives a concise high-level overview of
the implementation: describe the dotfile policy enforcement, how HTTP range
requests are parsed/handled (including partial vs full responses), the
index-file fallback behavior for directory requests, and the serializer buffer
protocol used when streaming files (buffer sizes, lifetime/ownership
expectations). Reference the top-level symbols serve_static (or the module-level
functions/types in this file) and any helpers that implement range
parsing/streaming so readers know which pieces of code the overview applies to.
| void await_suspend( | ||
| capy::coro cont, | ||
| capy::executor_ref caller_ex, | ||
| std::stop_token) | ||
| std::coroutine_handle<void> cont, | ||
| capy::io_env const* env) | ||
| { | ||
| auto caller_ex = env->executor; | ||
| auto& pool = capy::get_system_context(); | ||
| auto sys_ex = pool.get_executor(); | ||
| capy::run_async(sys_ex, | ||
| [this, cont, caller_ex] | ||
| (result r) mutable | ||
| { | ||
| result_ = r; | ||
| caller_ex.dispatch(cont); | ||
| caller_ex.dispatch(cont).resume(); | ||
| }, | ||
| [this, cont, caller_ex] | ||
| (std::exception_ptr ep) mutable | ||
| { | ||
| ep_ = ep; | ||
| caller_ex.dispatch(cont); | ||
| caller_ex.dispatch(cont).resume(); | ||
| } | ||
| )(hash_task(password_, rounds_, ver_)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and read the relevant sections
wc -l include/boost/http/bcrypt.hppRepository: cppalliance/http
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Read lines around 533-553 (await_suspend) and 580 (compare_async_op)
sed -n '530,560p' include/boost/http/bcrypt.hpp | cat -nRepository: cppalliance/http
Length of output: 1103
🏁 Script executed:
#!/bin/bash
# Check lines around 580 for compare_async_op
sed -n '575,600p' include/boost/http/bcrypt.hpp | cat -nRepository: cppalliance/http
Length of output: 956
🏁 Script executed:
#!/bin/bash
# Search for the function signatures and any documentation about io_env
rg -n 'await_suspend|compare_async_op' include/boost/http/bcrypt.hpp -B 3 -A 1Repository: cppalliance/http
Length of output: 638
🏁 Script executed:
#!/bin/bash
# Search for io_env usage patterns to understand if it can be null
rg -n 'io_env' include/boost/http/bcrypt.hpp -B 2 -A 2Repository: cppalliance/http
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Search for how hash_async and compare_async are used (call sites)
rg -n 'hash_async|compare_async' include/boost/http/bcrypt.hpp -A 5Repository: cppalliance/http
Length of output: 1624
🏁 Script executed:
#!/bin/bash
# Look for any co_await usage in the codebase to understand context
rg -n 'co_await' . --type=cpp | head -20Repository: cppalliance/http
Length of output: 1777
🏁 Script executed:
#!/bin/bash
# Check if there are other await_suspend implementations for comparison
rg -n 'await_suspend' . --type=cpp -B 3 -A 8 | head -50Repository: cppalliance/http
Length of output: 1594
🏁 Script executed:
#!/bin/bash
# Search for io_env usage patterns and whether it can be null
rg -n 'io_env' . --type=cpp | grep -E '(null|nullptr|if|check)' | head -20Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for any guard conditions around await_suspend or env parameter
fd -e hpp -e cpp | xargs grep -l 'await_suspend' | head -5Repository: cppalliance/http
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for test usage of hash_async and compare_async
fd -e cpp test | xargs grep -l 'hash_async\|compare_async'Repository: cppalliance/http
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check bcrypt tests to see context where these are called
find . -path '*test*' -name '*bcrypt*' -type fRepository: cppalliance/http
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Search for any documentation or other uses of io_env parameter contract
rg -n 'io_env' . --type=cpp -B 2 -A 2 | grep -E '(null|nullptr|guarantee|contract|non-null)' -iRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for how capy framework is documented or used
fd -e hpp include/boost/capy | head -10Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if capy provides any guard or context management for io_env
rg -n 'io_env' include/boost/capy --type=hpp -A 3 | head -40Repository: cppalliance/http
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Read the bcrypt test file to understand usage context
cat test/unit/bcrypt.cppRepository: cppalliance/http
Length of output: 15438
🏁 Script executed:
#!/bin/bash
# Search for any co_await context or io_env setup in tests
rg -n 'hash_async|compare_async' test/ -B 10 -A 5Repository: cppalliance/http
Length of output: 5648
🏁 Script executed:
#!/bin/bash
# Check if there are any other two-parameter await_suspend implementations
rg -n 'await_suspend' . --type=cpp -B 5 | grep -E '(await_suspend|const.*\*|nullptr|null)' | head -40Repository: cppalliance/http
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Search for capy documentation or examples about await_suspend contract
find . -name "*.hpp" -o -name "*.cpp" | xargs grep -l 'await_suspend' | xargs grep -l 'capy'Repository: cppalliance/http
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Look for any capy framework notes about io_env requirements
rg -n 'io_env' . --type=cpp --type=hpp | grep -E '(nullptr|null|require|guarantee|contract|must|non-null)' -iRepository: cppalliance/http
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Check if there are defensive null checks in similar code patterns
rg -n 'if.*env' include/boost/http/ --type=cpp --type=hppRepository: cppalliance/http
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for web references or external docs about capy await_suspend contract
grep -r "capy" README.md doc/ 2>/dev/null | head -20Repository: cppalliance/http
Length of output: 1967
🏁 Script executed:
#!/bin/bash
# Check if there's any CMakeLists or dependency info about capy
find . -name "CMakeLists.txt" -o -name "*.txt" | xargs grep -i capy 2>/dev/nullRepository: cppalliance/http
Length of output: 433
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation near the await_suspend definitions
sed -n '500,540p' include/boost/http/bcrypt.hppRepository: cppalliance/http
Length of output: 943
🏁 Script executed:
#!/bin/bash
# Look for the complete struct definitions and any surrounding documentation
sed -n '520,562p' include/boost/http/bcrypt.hppRepository: cppalliance/http
Length of output: 1079
🏁 Script executed:
#!/bin/bash
# Check the second function as well
sed -n '564,605p' include/boost/http/bcrypt.hppRepository: cppalliance/http
Length of output: 1069
🏁 Script executed:
#!/bin/bash
# Search for any similar two-parameter await_suspend in other Boost libraries
find . -name "*.hpp" -o -name "*.cpp" | xargs grep -l 'await_suspend' 2>/dev/nullRepository: cppalliance/http
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for capy::io_env definition or contract information
find . -name "io_env.hpp" -o -name "*io_env*" 2>/dev/null | head -5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if capy is vendored or an external dependency
ls -la include/boost/capy/ 2>/dev/null | head -20Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about two-parameter await_suspend
sed -n '500,530p' include/boost/http/bcrypt.hppRepository: cppalliance/http
Length of output: 667
🏁 Script executed:
#!/bin/bash
# Look at the public API documentation for hash_async and compare_async
sed -n '610,650p' include/boost/http/bcrypt.hppRepository: cppalliance/http
Length of output: 1121
🏁 Script executed:
#!/bin/bash
# Check if there's any `@requires` or precondition documentation
sed -n '600,680p' include/boost/http/bcrypt.hppRepository: cppalliance/http
Length of output: 1999
🏁 Script executed:
#!/bin/bash
# Search for any documentation about capy::io_env contract or preconditions
grep -r "io_env" doc/ README.md 2>/dev/null | grep -i "require\|precond\|contract\|must\|non-null" -iRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there are other async operations in the codebase with similar patterns
find . -name "*.hpp" -o -name "*.cpp" | xargs grep -l 'await_suspend' 2>/dev/null | xargs grep -A 5 'await_suspend'Repository: cppalliance/http
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Look for error handling patterns or null checks in capy usage
rg -n 'env' include/boost/http/ -A 2 | grep -E '(nullptr|if|check)' | head -20Repository: cppalliance/http
Length of output: 42
Add null-check or document the requirement that these awaitables must be co_await'd only within a capy io_env context.
env->executor at lines 537 and 580 dereferences env without validation. While the API is designed for capy coroutines where the framework provides io_env, there is no documented precondition preventing misuse. If either hash_async or compare_async awaitables are co_await'd outside a capy context, env could be null, causing undefined behavior. Either add a null-check with a descriptive error, or prominently document that these awaitables require execution within a capy io_env context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/http/bcrypt.hpp` around lines 533 - 553, The await_suspend
method currently dereferences env (env->executor) without validation, which can
crash if the awaitable is used outside a capy io_env; in await_suspend (the
lambda using caller_ex and the calls to caller_ex.dispatch(cont).resume()) add a
null-check for env (or env->executor) and handle the error by capturing/setting
ep_ to a descriptive std::runtime_error (or call cont.resume on the caller with
an exception) so misuse fails fast and clearly, or alternatively document in the
header that await_suspend (and the hash_async/compare_async awaitables that call
hash_task/compare_task) require being co_awaited inside a capy io_env. Ensure
the check references await_suspend, env->executor, and the
hash_task/compare_task call sites so reviewers can locate the change.
| capy::mutable_buffer arr[1]; | ||
| auto bufs = rp.res_body.prepare(arr); | ||
| if(bufs.empty()) | ||
| { | ||
| auto [ec2] = co_await rp.res_body.commit(0); | ||
| if(ec2) | ||
| co_return route_error(ec2); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.hpp" -o -name "*.cpp" | head -20Repository: cppalliance/http
Length of output: 822
🏁 Script executed:
fd -e hpp -e cpp "serve_static" | head -10Repository: cppalliance/http
Length of output: 164
🏁 Script executed:
git ls-files | grep -E "(serve_static|capy)" | head -20Repository: cppalliance/http
Length of output: 164
🏁 Script executed:
wc -l src/server/serve_static.cppRepository: cppalliance/http
Length of output: 91
🏁 Script executed:
sed -n '290,340p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 1297
🏁 Script executed:
rg -n "prepare\s*\(" include/ --type cpp -B 2 -A 10 | head -80Repository: cppalliance/http
Length of output: 4922
🏁 Script executed:
rg -n "commit\s*\(" include/ --type cpp -B 2 -A 10 | head -80Repository: cppalliance/http
Length of output: 4789
🏁 Script executed:
sed -n '370,400p' include/boost/http/serializer.hppRepository: cppalliance/http
Length of output: 834
🏁 Script executed:
sed -n '620,680p' include/boost/http/serializer.hppRepository: cppalliance/http
Length of output: 1630
🏁 Script executed:
rg -n "stream_prepare" include/boost/http/ -B 2 -A 15 | head -100Repository: cppalliance/http
Length of output: 6486
🏁 Script executed:
grep -n "need_data" include/boost/http/error.hppRepository: cppalliance/http
Length of output: 77
🏁 Script executed:
sed -n '620,650p' include/boost/http/serializer.hppRepository: cppalliance/http
Length of output: 911
🏁 Script executed:
rg -n "prepare.*empty" include/boost/http/ --type cpp -B 3 -A 3Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
sed -n '655,680p' include/boost/http/serializer.hppRepository: cppalliance/http
Length of output: 685
🏁 Script executed:
rg -n "while.*remaining" src/server/serve_static.cpp -B 5 -A 30Repository: cppalliance/http
Length of output: 1237
🏁 Script executed:
rg -n "commit.*0" test/ --type cpp -B 3 -A 3 | head -60Repository: cppalliance/http
Length of output: 3037
🏁 Script executed:
sed -n '645,680p' include/boost/http/serializer.hppRepository: cppalliance/http
Length of output: 907
🏁 Script executed:
rg -n "stream_capacity" include/boost/http/serializer.hpp -B 2 -A 10Repository: cppalliance/http
Length of output: 1684
🏁 Script executed:
rg -n "stream_prepare\|stream_commit" src/ include/ --type cpp -B 2 -A 8 | grep -A 8 -B 2 "empty"Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
sed -n '1,100p' include/boost/http/serializer.hpp | head -60Repository: cppalliance/http
Length of output: 1823
🏁 Script executed:
rg -n "is_done" include/boost/http/serializer.hpp -B 2 -A 3Repository: cppalliance/http
Length of output: 4443
🏁 Script executed:
rg -n "prepare.*arr\|prepare.*empty" src/server/ include/boost/http/server/ --type cpp -B 3 -A 5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
sed -n '700,730p' include/boost/http/serializer.hppRepository: cppalliance/http
Length of output: 810
🏁 Script executed:
python3 << 'EOF'
# Analyze the serve_static pattern
code = """
while(remaining > 0)
{
capy::mutable_buffer arr[1];
auto bufs = rp.res_body.prepare(arr);
if(bufs.empty())
{
auto [ec2] = co_await rp.res_body.commit(0);
if(ec2)
co_return route_error(ec2);
continue;
}
auto const n1 = f.read(bufs[0].data(), to_read, ec);
if(ec.failed() || n1 == 0)
break;
auto [ec2] = co_await rp.res_body.commit(n1);
if(ec2)
co_return route_error(ec2);
remaining -= static_cast<std::int64_t>(n1);
}
"""
print("Control flow analysis:")
print("1. Outer loop condition: while(remaining > 0)")
print("2. 'remaining' is only decremented on line 324 (normal path)")
print("3. When bufs.empty() is true (line 305):")
print(" - commit(0) is awaited")
print(" - continue is called (no 'remaining' decrement)")
print("4. Question: Can remaining stay the same while bufs.empty() repeatedly?")
print(" - Yes, if commit(0) doesn't make buffer space available")
print(" - This would cause unbounded retries at await points")
EOFRepository: cppalliance/http
Length of output: 494
🏁 Script executed:
rg -n "stream_prepare\(\)" src/ include/ --type cpp -B 5 -A 10 | head -100Repository: cppalliance/http
Length of output: 4804
🏁 Script executed:
sed -n '588,615p' src/serializer.cppRepository: cppalliance/http
Length of output: 603
🏁 Script executed:
rg -n "out_prepare\|out_capacity" src/serializer.cpp -B 3 -A 8 | head -80Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
grep -n "stall\|timeout\|retry" src/server/serve_static.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
sed -n '655,680p' include/boost/http/serializer.hppRepository: cppalliance/http
Length of output: 685
🏁 Script executed:
sed -n '540,580p' src/serializer.cppRepository: cppalliance/http
Length of output: 979
🏁 Script executed:
rg -n "out_prepare\|out_commit" src/serializer.cpp -B 2 -A 10 | head -120Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
sed -n '280,320p' src/serializer.cppRepository: cppalliance/http
Length of output: 974
🏁 Script executed:
sed -n '560,600p' src/serializer.cppRepository: cppalliance/http
Length of output: 947
🏁 Script executed:
rg -n "out_prepare" src/serializer.cpp -B 5 -A 15Repository: cppalliance/http
Length of output: 2454
🏁 Script executed:
sed -n '450,500p' src/serializer.cppRepository: cppalliance/http
Length of output: 1806
🏁 Script executed:
sed -n '671,690p' src/serializer.cppRepository: cppalliance/http
Length of output: 491
🏁 Script executed:
rg -n "capy::buffer_pair\|out_\|in_" src/serializer.cpp | head -40Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
python3 << 'EOF'
# Trace the flow when prepare() returns empty in serve_static
print("=== Scenario: prepare() returns empty ===")
print()
print("serve_static (lines 305-310):")
print(" auto bufs = rp.res_body.prepare(arr); // Returns empty span")
print(" if(bufs.empty()) {")
print(" auto [ec2] = co_await rp.res_body.commit(0);")
print(" continue;")
print(" }")
print()
print("sink.prepare() (serializer.hpp:622-635):")
print(" auto bufs = sr_->stream_prepare(); // Returns mutable_buffer_pair")
print(" // Loop fills dest[0] if bufs[0].size() > 0, else breaks")
print(" return dest.first(0); // Empty result if all bufs have size 0")
print()
print("sink.commit() (serializer.hpp:648-680):")
print(" sr_->stream_commit(0); // Commits 0 bytes")
print(" while(!sr_->is_done()) {")
print(" auto cbs = sr_->prepare(); // Calls sr_->prepare()")
print(" if(capy::buffer_empty(*cbs)) {")
print(" sr_->consume(0); // Advances state machine")
print(" continue;")
print(" }")
print(" auto [ec, written] = co_await stream_->write_some(*cbs);")
print(" sr_->consume(written);")
print(" if(ec) co_return {ec};")
print(" }")
print(" co_return {}; // Returns successfully")
print()
print("Key question:")
print(" If stream_prepare() initially returned empty buffers,")
print(" how does is_done() become true?")
print(" - If is_done() is already true, the while loop never executes")
print(" - If is_done() is false, loop executes sr_->prepare()")
print(" which may still return empty buffers")
print(" causing consume(0) and continue indefinitely")
print()
print(" The critical path: Does sr_->prepare()'s behavior change")
print(" after sr_->consume(0) is called?")
EOFRepository: cppalliance/http
Length of output: 1368
🏁 Script executed:
sed -n '671,700p' src/serializer.cppRepository: cppalliance/http
Length of output: 789
🏁 Script executed:
sed -n '684,720p' src/serializer.cppRepository: cppalliance/http
Length of output: 908
🏁 Script executed:
sed -n '200,250p' src/serializer.cppRepository: cppalliance/http
Length of output: 970
🏁 Script executed:
rg -n "is_done\|state_" src/serializer.cpp | head -50Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
python3 << 'EOF'
# Analyze the actual behavior when prepare() returns empty
print("=== Control flow when prepare(arr) returns empty ===\n")
print("Scenario: serve_static calls prepare(arr) and gets empty result")
print()
print("1. sink.prepare(arr) at line 304:")
print(" - calls sr_->stream_prepare()")
print(" - if stream_prepare() returns empty buffers (because out_.capacity() == 0)")
print(" - prepare(arr) returns dest.first(0) → empty span")
print()
print("2. sink.commit(0) at line 307:")
print(" - calls sr_->stream_commit(0)")
print(" - for chunked: if(n == 0) return; → early return, no state change!")
print(" - then enters while(!sr_->is_done()) loop")
print(" - calls sr_->prepare() [the serializer's prepare, not stream_prepare]")
print(" - this tries to flush internal serialization state")
print(" - calls write_some() to send data to network")
print(" - eventually exits when is_done() is true")
print()
print("3. Back in serve_static, next iteration:")
print(" - calls prepare(arr) again")
print(" - but stream_prepare() might STILL return empty")
print(" - because write_some() in the background may not have completed yet")
print()
print("KEY ISSUE:")
print("- serve_static loop is unbounded")
print("- No retry counter, no timeout")
print("- Relies entirely on write_some() making progress")
print("- If the network buffer fills and write_some() stalls...")
print(" the entire coroutine hangs without timeout")
EOFRepository: cppalliance/http
Length of output: 1198
🏁 Script executed:
rg -n "while.*remaining\|while.*!.*done" src/server/serve_static.cpp -B 2 -A 8Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
sed -n '1,50p' src/server/serve_static.cppRepository: cppalliance/http
Length of output: 1271
🏁 Script executed:
cat test/unit/server/serve_static.cpp | head -100Repository: cppalliance/http
Length of output: 435
🏁 Script executed:
rg -n "prepare.*empty\|commit.*0" test/unit/server/serve_static.cpp -B 3 -A 5 | head -80Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
python3 << 'EOF'
# Final analysis: is the review comment valid?
print("=== Final Analysis ===\n")
print("CLAIM: Unbounded retry when prepare() returns empty")
print()
print("Evidence found:")
print("1. serve_static loop condition: while(remaining > 0)")
print(" - No iteration cap or timeout")
print()
print("2. When prepare() returns empty (line 305):")
print(" - Calls co_await commit(0)")
print(" - Calls continue (no remaining decrement)")
print()
print("3. Inside commit() (serializer.hpp:648-680):")
print(" - Calls sr_->stream_commit(0)")
print(" - Enters while(!sr_->is_done()) draining loop")
print(" - Calls write_some() to flush buffers to network")
print(" - Exits when is_done() true")
print()
print("4. Key mechanism for forward progress:")
print(" - commit(0) is co_await, so it yields control")
print(" - The draining loop flushes internal buffers via write_some()")
print(" - write_some() is async and processes network events")
print(" - Once space is freed, stream_prepare() should return non-empty")
print()
print("VERDICT on original review:")
print("- The concern about unbounded retries is PARTIALLY VALID")
print("- However, the code relies on async progress via write_some()")
print("- The loop is NOT truly 'unbounded' in a blocking sense")
print("- It yields control at await points, allowing network progress")
print()
print("BUT: The original review's core concern remains valid:")
print("- No explicit safeguard (timeout, iteration limit)")
print("- Relies on implicit assumption that write_some() makes progress")
print("- If network stalls, the loop could hang indefinitely")
print("- This is a valid design concern worth addressing")
EOFRepository: cppalliance/http
Length of output: 1389
Clarify forward progress mechanism for prepare() when returning empty buffers.
When rp.res_body.prepare(arr) returns empty (internal buffer full), the code calls co_await commit(0) and retries. This loop has no explicit iteration cap or timeout. While commit(0) yields control via co_await (allowing async network operations to drain the buffer), forward progress depends entirely on the internal write_some() flushing data successfully. Explicitly document or verify that capy's async machinery guarantees buffer space becomes available after a successful commit(0) call. If that guarantee cannot be relied upon, add an iteration limit or explicit timeout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/serve_static.cpp` around lines 303 - 311, The loop that calls
rp.res_body.prepare(arr) and, on empty result, awaits rp.res_body.commit(0)
lacks a forward-progress cap; update serve_static.cpp to either document the
guarantee from capy that a successful co_await rp.res_body.commit(0) will make
space (i.e., commit triggers underlying write_some and frees buffer) or
implement a bounded retry/timeout: add a max retry counter (e.g.,
MAX_PREPARE_RETRIES) around the prepare/commit loop, after which return
route_error or a timeout error, and optionally insert a short async
backoff/yield between commits; ensure you reference and modify the loop that
uses capy::mutable_buffer arr[], rp.res_body.prepare(...), and
rp.res_body.commit(0) and return route_error(ec) when the retry limit or timeout
is reached.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 245: The changelog job still uses the old step name "Clone Boost.Corosio"
which is inconsistent with the other jobs; update the step name in the changelog
job (the job named "changelog") to "Clone Boost.Http" so it matches the build
and antora jobs and keeps step names consistent across the workflow.
- Around line 278-279: The CI workflow uses the YAML folding scalar (">") for
the patches key which collapses newlines into spaces and breaks the expected
string[] input; update the patches parameter to use the literal block scalar
("|") at the current occurrence and the similar occurrence in the antora job so
each repository URL remains on its own line under the patches key (ensure each
URL is placed on a separate line after patches: |).
| patches: > | ||
| https://github.com/cppalliance/capy |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the full context around lines 278-279 in the workflow
sed -n '270,290p' .github/workflows/ci.ymlRepository: cppalliance/http
Length of output: 676
🏁 Script executed:
# Also check the antora job usage mentioned in the scratchpad
sed -n '520,535p' .github/workflows/ci.ymlRepository: cppalliance/http
Length of output: 532
🏁 Script executed:
# Search for boost-clone action definition or usage examples in the repo
rg "boost-clone" --type yaml -A 5 -B 5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Look for any documentation about the patches parameter
rg "patches:" --type yaml -B 2 -A 2Repository: cppalliance/http
Length of output: 42
🌐 Web query:
boost-clone action alandefreitas cpp-actions patches parameter format
💡 Result:
In alandefreitas/cpp-actions/boost-clone, the patches input is a string[] (list of strings). Each entry should be a Git repository URL for a “patch” Boost-library repo; the action will clone each repo into BOOST_DIR/libs/<repo-name> (where <repo-name> is taken from the URL) using the same branch you set for the Boost superproject. [1][2]
Typical workflow format (multi-line list):
- uses: alandefreitas/cpp-actions/boost-clone@master
with:
branch: develop
patches: |
https://github.com/your-org/boost-my_library.git
https://github.com/your-org/boost-another_lib.git(Each line becomes one element of the string[].) [1][2]
Notes
- You can also pass a single patch (one line). [1]
- The action clones patches with
cloneGitRepo(patch, patchDir, inputs.branch), i.e., it uses yourbranchvalue for those patch repos too. [2]
Sources: Boost Clone docs + schema/type + patch-clone implementation. [1][2]
References
- Boost Clone Action docs / inputs (
patches) — https://alandefreitas.github.io/cpp-actions/cpp-actions/actions/boost-clone.html boost-cloneschema +applyPatches()implementation — https://raw.githubusercontent.com/alandefreitas/cpp-actions/master/boost-clone/src/schema.ts , https://raw.githubusercontent.com/alandefreitas/cpp-actions/master/boost-clone/src/git-utils.ts
🏁 Script executed:
# Try to fetch the action definition from the upstream repository
# Check if we can get info about the patches parameter
curl -s https://raw.githubusercontent.com/alandefreitas/cpp-actions/v1.9.3/action/boost-clone/action.yml | head -150Repository: cppalliance/http
Length of output: 74
Use | (literal block scalar) instead of > (folding scalar) for the patches parameter.
The patches input expects a string[] where each line is a separate Git repository URL. Using > folds newlines into spaces, breaking the list parsing. Change to | to preserve line breaks:
patches: |
https://github.com/cppalliance/capy(Same fix needed at line 527–528 in the antora job.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 278 - 279, The CI workflow uses the
YAML folding scalar (">") for the patches key which collapses newlines into
spaces and breaks the expected string[] input; update the patches parameter to
use the literal block scalar ("|") at the current occurrence and the similar
occurrence in the antora job so each repository URL remains on its own line
under the patches key (ensure each URL is placed on a separate line after
patches: |).
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 526-527: The antora job's "patches: >" uses a folded scalar which
collapses newlines into spaces; change it to a literal block scalar "patches: |"
so the patches input remains a newline-separated list of URLs (preserving line
breaks and matching the build job style), i.e., replace the ">" after the
"patches:" key with "|" to ensure each URL stays on its own line.
ashtum
force-pushed
the
fix-build
branch
2 times, most recently
from
7421ddf to
4e50d5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 276: Replace the invalid hyphen value for the modules-exclude-paths
parameter with an empty string (or remove the parameter) so it matches the
antora job's override; locate the modules-exclude-paths entry in the CI workflow
(the build job) and change modules-exclude-paths: '-' to modules-exclude-paths:
'' (or delete the line) to provide a valid empty list override.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 278-279: The CI workflow uses a folded scalar for the patches
input ("patches: >") which collapses newlines into spaces and can break
multi-URL lists; change the YAML scalar to a literal block ("patches: |") for
the affected keys in both the build job (current occurrence at "patches: >") and
the antora job (the other occurrence around lines 527–528) so each URL remains
on its own line as the patches input expects a string[].
- Line 485: The step name in the changelog GitHub Actions job is still "Clone
Boost.Corosio" and must match the other jobs; update the step name string in the
changelog job from "Clone Boost.Corosio" to "Clone Boost.Http" so it is
consistent with the `build` and `antora` jobs (search for the step named "Clone
Boost.Corosio" inside the `changelog` job and replace the name value).
334067f to
7c67dd4
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 278-279: In the CI workflow YAML update the "patches:" scalar
style from a folded scalar (>) to a literal scalar (|) so newline characters are
preserved; locate the "patches:" key under the job where it currently uses ">"
(and the duplicate occurrence in the antora job) and replace the ">" with "|" so
each patch URL remains on its own line and additional URLs won't be collapsed.
In `@src/server/serve_static.cpp`:
- Around line 303-311: Add a concise inline comment in the loop around
rp.res_body.prepare(arr) and the co_await rp.res_body.commit(0) retry path
explaining the forward-progress guarantee: note that when prepare() returns an
empty span we intentionally call commit(0) to flush the serializer (via its
internal write_some) which will free buffer space and allow the next prepare()
to succeed, so the loop can safely continue without decrementing remaining;
place this comment adjacent to the prepare()/commit(0) code to make the
rationale for the retry explicit.
2 participants
Summary by CodeRabbit
Refactor
Chores