Properly response to HEAD with Content-Length on compressed content#1040
Merged
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: Improves compliance for Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| // precomputed compressed size, so we can answer HEAD without | ||
| // running the compressor just to discard its output. | ||
| if (method == "head" && precomputed_compressed_size.has_value()) { | ||
| this->response_->endWithoutBody(precomputed_compressed_size); |
There was a problem hiding this comment.
endWithoutBody is called with precomputed_compressed_size (an std::optional<std::size_t>), which likely needs the contained numeric value; otherwise this risks a compile error and/or an incorrect Content-Length for HEAD responses.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/metapack/include/sourcemeta/one/metapack.h">
<violation number="1" location="src/metapack/include/sourcemeta/one/metapack.h:22">
P2: Changing `METAPACK_VERSION` to 2 without a backward-compatible v1 read path makes existing metapack artifacts unreadable at runtime.</violation>
</file>
<file name="src/metapack/metapack.cc">
<violation number="1" location="src/metapack/metapack.cc:82">
P2: Identity metapack writes now hard-depend on gzip success, introducing a new failure path for non-gzip storage.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| static constexpr std::uint32_t METAPACK_MAGIC{0x4154454D}; | ||
| static constexpr std::uint16_t METAPACK_VERSION{1}; | ||
| static constexpr std::uint16_t METAPACK_VERSION{2}; |
There was a problem hiding this comment.
P2: Changing METAPACK_VERSION to 2 without a backward-compatible v1 read path makes existing metapack artifacts unreadable at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/metapack/include/sourcemeta/one/metapack.h, line 22:
<comment>Changing `METAPACK_VERSION` to 2 without a backward-compatible v1 read path makes existing metapack artifacts unreadable at runtime.</comment>
<file context>
@@ -19,7 +19,7 @@
static constexpr std::uint32_t METAPACK_MAGIC{0x4154454D};
-static constexpr std::uint16_t METAPACK_VERSION{1};
+static constexpr std::uint16_t METAPACK_VERSION{2};
static constexpr std::uint64_t METAPACK_MAX_DECOMPRESSION_RATIO{1024};
</file context>
| // so a future codec swap is mechanical). For compressed-storage | ||
| // artifacts the bytes are also what we write to disk; for Identity | ||
| // storage only the size is kept. | ||
| const auto compressed{sourcemeta::core::gzip( |
There was a problem hiding this comment.
P2: Identity metapack writes now hard-depend on gzip success, introducing a new failure path for non-gzip storage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/metapack/metapack.cc, line 82:
<comment>Identity metapack writes now hard-depend on gzip success, introducing a new failure path for non-gzip storage.</comment>
<file context>
@@ -72,14 +74,18 @@ static auto write_metapack(const std::filesystem::path &destination,
+ // so a future codec swap is mechanical). For compressed-storage
+ // artifacts the bytes are also what we write to disk; for Identity
+ // storage only the size is kept.
+ const auto compressed{sourcemeta::core::gzip(
+ reinterpret_cast<const std::uint8_t *>(content.data()), content.size())};
sourcemeta::core::write_file(destination, [&](std::ostream &output) {
</file context>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: e44ae13 | Previous: de8d2e4 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
343 ms |
391 ms |
0.88 |
Add one schema (100 existing) |
22 ms |
30 ms |
0.73 |
Add one schema (1000 existing) |
74 ms |
90 ms |
0.82 |
Add one schema (10000 existing) |
570 ms |
734 ms |
0.78 |
Update one schema (1 existing) |
16 ms |
22 ms |
0.73 |
Update one schema (101 existing) |
23 ms |
29 ms |
0.79 |
Update one schema (1001 existing) |
68 ms |
91 ms |
0.75 |
Update one schema (10001 existing) |
604 ms |
747 ms |
0.81 |
Cached rebuild (1 existing) |
4 ms |
7 ms |
0.57 |
Cached rebuild (101 existing) |
5 ms |
9 ms |
0.56 |
Cached rebuild (1001 existing) |
26 ms |
31 ms |
0.84 |
Cached rebuild (10001 existing) |
161 ms |
274 ms |
0.59 |
Index 100 schemas |
560 ms |
556 ms |
1.01 |
Index 1000 schemas |
1299 ms |
1574 ms |
0.83 |
Index 10000 schemas |
11627 ms |
13551 ms |
0.86 |
Index 10000 schemas (custom meta-schema) |
136684 ms |
133514 ms |
1.02 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: e44ae13 | Previous: de8d2e4 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
377 ms |
291 ms |
1.30 |
Add one schema (100 existing) |
33 ms |
26 ms |
1.27 |
Add one schema (1000 existing) |
94 ms |
82 ms |
1.15 |
Add one schema (10000 existing) |
878 ms |
795 ms |
1.10 |
Update one schema (1 existing) |
25 ms |
19 ms |
1.32 |
Update one schema (101 existing) |
31 ms |
25 ms |
1.24 |
Update one schema (1001 existing) |
95 ms |
90 ms |
1.06 |
Update one schema (10001 existing) |
789 ms |
837 ms |
0.94 |
Cached rebuild (1 existing) |
8 ms |
6 ms |
1.33 |
Cached rebuild (101 existing) |
10 ms |
8 ms |
1.25 |
Cached rebuild (1001 existing) |
33 ms |
27 ms |
1.22 |
Cached rebuild (10001 existing) |
279 ms |
220 ms |
1.27 |
Index 100 schemas |
576 ms |
571 ms |
1.01 |
Index 1000 schemas |
1562 ms |
1306 ms |
1.20 |
Index 10000 schemas |
13657 ms |
12746 ms |
1.07 |
Index 10000 schemas (custom meta-schema) |
131797 ms |
104594 ms |
1.26 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/router/artifact.cc">
<violation number="1" location="src/router/artifact.cc:347">
P2: Debug-only `assert()` guards a narrowing `static_cast`—the check disappears in release builds, so a >4 GB `compressed_bytes` on a 32-bit build would silently truncate and produce an incorrect `Content-Length` in HEAD responses.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // and the assert guards a hypothetical 32-bit build from truncating | ||
| // a >4 GB artifact (which the indexer would never produce on | ||
| // realistic schema inputs). | ||
| assert(info->compressed_bytes <= std::numeric_limits<std::size_t>::max()); |
There was a problem hiding this comment.
P2: Debug-only assert() guards a narrowing static_cast—the check disappears in release builds, so a >4 GB compressed_bytes on a 32-bit build would silently truncate and produce an incorrect Content-Length in HEAD responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/router/artifact.cc, line 347:
<comment>Debug-only `assert()` guards a narrowing `static_cast`—the check disappears in release builds, so a >4 GB `compressed_bytes` on a 32-bit build would silently truncate and produce an incorrect `Content-Length` in HEAD responses.</comment>
<file context>
@@ -336,6 +338,13 @@ auto RouterAction::artifact_serve(
+ // and the assert guards a hypothetical 32-bit build from truncating
+ // a >4 GB artifact (which the indexer would never produce on
+ // realistic schema inputs).
+ assert(info->compressed_bytes <= std::numeric_limits<std::size_t>::max());
sourcemeta::one::send_response(
status, request, response, contents,
</file context>
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
1 participant
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.
Signed-off-by: Juan Cruz Viotti jv@jviotti.com