fix: guard engine hook span allocation

shyim · shyim · commit c1bc8ee99b2c · 2026-05-24T13:09:24.000+02:00
diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md
@@ -52,6 +52,8 @@ Validation performed on macOS/PHP 8.5.6:
   Result: **7 passed, 0 failed**.
 - `TEST_PHP_EXECUTABLE=$(which php) make test NO_INTERACTION=1 TESTS='tests/221-twig-long-block-name-clamps-buffer.phpt tests/015-long-function-names.phpt tests/214-twig-template-name.phpt tests/215-twig-wrapper-template-name.phpt tests/216-symfony-event-dispatch-name.phpt tests/201-io-spans-with-trace-functions-off.phpt tests/205-many-io-spans.phpt'`
   Result: **7 passed, 0 failed**.
+- `TEST_PHP_EXECUTABLE=$(which php) make test NO_INTERACTION=1 TESTS='tests/222-engine-hooks-capacity-smoke.phpt tests/084-otlp-runtime-env.phpt tests/080-otlp-valid-json.phpt tests/102-span-capacity-grows.phpt tests/217-exception-status-first-span.phpt tests/218-log-exception-status-first-span.phpt'`
+  Result: **6 passed, 0 failed**.
 
 ### Critical / High Follow-up Status
 
@@ -74,7 +76,7 @@ Validation performed on macOS/PHP 8.5.6:
 |---------|--------|-------|
 | Manual spans are never finished | **Fixed** | Manual spans are marked and finalized during request shutdown finalization before UDP export. `createSpan()` parent lookup now uses the validated current-span helper that skips sentinels. Covered by `tests/219-manual-span-parent-skips-sentinel.phpt` and `tests/220-manual-span-exported-on-disable.phpt`. |
 | cURL hook uses private ext/curl layout and leaks `curl_slist` | **Open / partial** | Header restore support exists, but the hook still relies on private ext/curl layout and restored `curl_slist`s are not retained/freed after `curl_easy_setopt()`. `curl_setopt_array()` is still not handled. |
-| Engine hooks ignore capacity failures | **Open** | Compile/GC hooks still call `ensure_span_capacity(state)` and increment `span_count` even if capacity fails. |
+| Engine hooks ignore capacity failures | **Fixed** | Compile/GC hooks now return early when `ensure_span_capacity(state)` fails, so `span_count` is only incremented after capacity is guaranteed. Engine db attrs also grow on demand. Covered by `tests/222-engine-hooks-capacity-smoke.phpt` and span capacity regression coverage. |
 | Global request state is not ZTS-safe | **Open** | `g_state` is still process-global. README currently claims full ZTS support, which does not match the implementation. |
 
 ### Tests, CI, and Packaging Status
@@ -86,15 +88,14 @@ Validation performed on macOS/PHP 8.5.6:
 | Optional integrations mostly skipped in default CI | **Open** | Redis, MySQLi, and Memcached tests still depend on local optional services/extensions and skipped locally. |
 | macOS/BSD portability is not covered by CI | **Open** | `.github/workflows/tests.yml` still only runs Ubuntu. |
 | Build config has implicit libcurl dependency | **Open** | `config.m4` still does not check curl headers/libraries despite `hook_curl.c` including `<curl/curl.h>`. |
-| README/demo drift | **Open** | README still documents `akari.mode=auto`, lists 74 PHPT tests while 89 exist, and demo README uses port 8081 while `docker-compose.yml` maps the PHP demo to 8083. |
+| README/demo drift | **Open** | README still documents `akari.mode=auto`, lists 74 PHPT tests while 95 exist, and demo README uses port 8081 while `docker-compose.yml` maps the PHP demo to 8083. |
 
 ### Remaining Remediation Order
 
-1. Fix engine hook capacity handling before `span_count++`.
-2. Replace network-backed curl PHPTs with local fixtures or real `--SKIPIF--` checks.
-3. Add CI jobs for `go test ./...`, macOS, and optional Redis/MySQL/Memcached integration tests.
-4. Add explicit libcurl checks to `config.m4`.
-5. Update README/demo claims to match current behavior.
+1. Replace network-backed curl PHPTs with local fixtures or real `--SKIPIF--` checks.
+2. Add CI jobs for `go test ./...`, macOS, and optional Redis/MySQL/Memcached integration tests.
+3. Add explicit libcurl checks to `config.m4`.
+4. Update README/demo claims to match current behavior.
 
 ## Executive Summary
 
diff --git a/src/hook_engine.c b/src/hook_engine.c
@@ -17,6 +17,46 @@
 static zend_op_array *(*original_compile_file)(zend_file_handle *file_handle, int type) = NULL;
 static int (*original_gc_collect_cycles)(void) = NULL;
 
+/* ── Engine attr helpers ── */
+
+static profiler_db_attr_t *engine_db_attr_alloc(profiler_state_t *state,
+                                                 uint32_t span_index,
+                                                 const char *system)
+{
+    if (state->db_attr_count >= state->db_attr_capacity) {
+        size_t new_cap = state->db_attr_capacity
+            ? state->db_attr_capacity * 2
+            : PROFILER_INITIAL_DB_ATTRS;
+        profiler_db_attr_t *new_attrs = realloc(state->db_attrs,
+            new_cap * sizeof(profiler_db_attr_t));
+        if (!new_attrs) return NULL;
+        state->db_attrs = new_attrs;
+        state->db_attr_capacity = new_cap;
+    }
+
+    profiler_db_attr_t *attr = &state->db_attrs[state->db_attr_count++];
+    memset(attr, 0, sizeof(profiler_db_attr_t));
+    attr->span_index = span_index;
+    snprintf(attr->db_system, sizeof(attr->db_system), "%s", system);
+    return attr;
+}
+
+static void engine_db_attr_set_statement(profiler_db_attr_t *attr,
+                                          const char *statement,
+                                          size_t statement_len)
+{
+    if (!attr || !statement || statement_len == 0) return;
+
+    size_t copy_len = statement_len;
+    if (copy_len >= sizeof(attr->db_statement)) {
+        copy_len = sizeof(attr->db_statement) - 1;
+    }
+
+    memcpy(attr->db_statement, statement, copy_len);
+    attr->db_statement[copy_len] = '\0';
+    attr->db_statement_len = copy_len;
+}
+
 /* ── Compile file hook ──
  *
  * Wraps zend_compile_file to measure compilation time per file.
@@ -37,48 +77,38 @@ static zend_op_array *profiler_compile_file(zend_file_handle *file_handle, int t
 
         /* Only record if compilation took >1ms */
         if (elapsed >= 1000000) {
-            ensure_span_capacity(state);
+            if (!ensure_span_capacity(state)) {
+                return result;
+            }
 
             size_t span_idx = state->span_count++;
-            if (span_idx < state->span_capacity) {
-                profiler_span_t *span = &state->spans[span_idx];
-                memset(span, 0, sizeof(profiler_span_t));
-
-                memcpy(span->trace_id, state->trace_id, 32);
-                profiler_generate_hex_id(state, span->span_id, 16);
-                span->start_time_ns = t_start;
-                span->end_time_ns = t_end;
-                span->depth = state->stack_depth;
-                span->kind = SPAN_KIND_INTERNAL;
-                span->status_code = SPAN_STATUS_UNSET;
-
-                /* Parent = current root if active */
-                if (state->root.active) {
-                    memcpy(span->parent_span_id, state->root.span_id, 16);
-                    span->has_parent = 1;
-                }
-
-                /* Record filename as attribute */
-                if (file_handle && file_handle->filename) {
-                    const char *fname = ZSTR_VAL(file_handle->filename);
-                    size_t fname_len = ZSTR_LEN(file_handle->filename);
-
-                    if (state->db_attr_count < state->db_attr_capacity) {
-                        profiler_db_attr_t *attr = &state->db_attrs[state->db_attr_count];
-                        memset(attr, 0, sizeof(profiler_db_attr_t));
-                        attr->span_index = (uint32_t)span_idx;
-                        snprintf(attr->db_system, DB_SYSTEM_MAX, "compile");
-                        size_t copy_len = fname_len;
-                        if (copy_len >= DB_STATEMENT_MAX) copy_len = DB_STATEMENT_MAX - 1;
-                        memcpy(attr->db_statement, fname, copy_len);
-                        attr->db_statement[copy_len] = '\0';
-                        attr->db_statement_len = copy_len;
-                        state->db_attr_count++;
-                    }
-                }
-
-                maybe_flush(state);
+            profiler_span_t *span = &state->spans[span_idx];
+            memset(span, 0, sizeof(profiler_span_t));
+
+            memcpy(span->trace_id, state->trace_id, 32);
+            profiler_generate_hex_id(state, span->span_id, 16);
+            span->start_time_ns = t_start;
+            span->end_time_ns = t_end;
+            span->depth = state->stack_depth;
+            span->kind = SPAN_KIND_INTERNAL;
+            span->status_code = SPAN_STATUS_UNSET;
+
+            /* Parent = current root if active */
+            if (state->root.active) {
+                memcpy(span->parent_span_id, state->root.span_id, 16);
+                span->has_parent = 1;
+            }
+
+            /* Record filename as attribute */
+            if (file_handle && file_handle->filename) {
+                profiler_db_attr_t *attr = engine_db_attr_alloc(state,
+                    (uint32_t)span_idx, "compile");
+                engine_db_attr_set_statement(attr,
+                    ZSTR_VAL(file_handle->filename),
+                    ZSTR_LEN(file_handle->filename));
             }
+
+            maybe_flush(state);
         }
     }
 
@@ -105,41 +135,39 @@ static int profiler_gc_collect_cycles(void)
 
         /* Only record if GC took >5ms (GC is typically fast) */
         if (elapsed >= 5000000) {
-            ensure_span_capacity(state);
+            if (!ensure_span_capacity(state)) {
+                return collected;
+            }
 
             size_t span_idx = state->span_count++;
-            if (span_idx < state->span_capacity) {
-                profiler_span_t *span = &state->spans[span_idx];
-                memset(span, 0, sizeof(profiler_span_t));
-
-                memcpy(span->trace_id, state->trace_id, 32);
-                profiler_generate_hex_id(state, span->span_id, 16);
-                span->start_time_ns = t_start;
-                span->end_time_ns = t_end;
-                span->depth = state->stack_depth;
-                span->kind = SPAN_KIND_INTERNAL;
-                span->status_code = SPAN_STATUS_UNSET;
-
-                if (state->root.active) {
-                    memcpy(span->parent_span_id, state->root.span_id, 16);
-                    span->has_parent = 1;
-                }
-
-                /* Record GC stats as attribute */
-                if (state->db_attr_count < state->db_attr_capacity) {
-                    profiler_db_attr_t *attr = &state->db_attrs[state->db_attr_count];
-                    memset(attr, 0, sizeof(profiler_db_attr_t));
-                    attr->span_index = (uint32_t)span_idx;
-                    snprintf(attr->db_system, DB_SYSTEM_MAX, "gc");
-                    snprintf(attr->db_statement, DB_STATEMENT_MAX,
-                        "collected %d roots in %.2fms",
-                        collected, elapsed / 1000000.0);
-                    attr->db_statement_len = strlen(attr->db_statement);
-                    state->db_attr_count++;
-                }
-
-                maybe_flush(state);
+            profiler_span_t *span = &state->spans[span_idx];
+            memset(span, 0, sizeof(profiler_span_t));
+
+            memcpy(span->trace_id, state->trace_id, 32);
+            profiler_generate_hex_id(state, span->span_id, 16);
+            span->start_time_ns = t_start;
+            span->end_time_ns = t_end;
+            span->depth = state->stack_depth;
+            span->kind = SPAN_KIND_INTERNAL;
+            span->status_code = SPAN_STATUS_UNSET;
+
+            if (state->root.active) {
+                memcpy(span->parent_span_id, state->root.span_id, 16);
+                span->has_parent = 1;
             }
+
+            /* Record GC stats as attribute */
+            profiler_db_attr_t *attr = engine_db_attr_alloc(state,
+                (uint32_t)span_idx, "gc");
+            if (attr) {
+                int n = snprintf(attr->db_statement, sizeof(attr->db_statement),
+                    "collected %d roots in %.2fms",
+                    collected, elapsed / 1000000.0);
+                attr->db_statement_len = profiler_clamp_snprintf_len(n,
+                    sizeof(attr->db_statement));
+            }
+
+            maybe_flush(state);
         }
     }
 
diff --git a/tests/222-engine-hooks-capacity-smoke.phpt b/tests/222-engine-hooks-capacity-smoke.phpt
@@ -0,0 +1,44 @@
+--TEST--
+Engine hooks: compile and GC profiling preserve span state
+--SKIPIF--
+<?php include __DIR__ . '/_skipif.inc'; ?>
+--INI--
+akari.enable=1
+akari.trace_functions=0
+akari.trace_internal=1
+akari.trace_compile=1
+akari.trace_gc=1
+--FILE--
+<?php
+$path = tempnam(sys_get_temp_dir(), 'akari_engine_hook_');
+$prefix = 'akari_engine_hook_' . getmypid() . '_';
+$code = "<?php\n";
+for ($i = 0; $i < 800; $i++) {
+    $code .= "function {$prefix}{$i}() { return {$i}; }\n";
+}
+file_put_contents($path, $code);
+include $path;
+unlink($path);
+
+gc_enable();
+$cycles = [];
+for ($i = 0; $i < 1000; $i++) {
+    $node = [];
+    $node['self'] = &$node;
+    $cycles[] = $node;
+}
+unset($cycles, $node);
+gc_collect_cycles();
+
+usleep(100);
+
+$json = Akari\getSpansJson();
+$data = json_decode($json, true);
+
+echo "valid json: " . (is_array($data) ? 'yes' : 'no') . "\n";
+$spans = $data['resourceSpans'][0]['scopeSpans'][0]['spans'] ?? [];
+echo "has spans: " . (count($spans) > 0 ? 'yes' : 'no') . "\n";
+?>
+--EXPECT--
+valid json: yes
+has spans: yes
