NAPI surface reduction and tightening

[syrusakbary]

This PR aims to simplify the API surface of the unofficial NAPI.

[Arshia001]

Read this alongside edgejs#147 (whose napi submodule pin 60c81ef is this branch's head). The consolidation is genuinely good work — collapsing the six env setters into one attach_env, the tagged bytecode_open transaction that owns cache validation and the fallback compile, typed unofficial_napi_module/unofficial_napi_message/unofficial_napi_profile handles, and the tagged unofficial_napi_js_source all remove classes of caller mistakes rather than just shrinking a count. The js_source_is_valid / js_source_from_text inline helpers in particular make the old {text, bytecode} two-nullable-fields footgun unrepresentable.

Main things I'd want addressed:

1. QuickJS process.memoryUsage().arrayBuffers regresses to 0 — the get_process_memory_info fold drops array_buffer_memory (and peak_malloced_memory) with no consumer-side compensation.
2. valid_fields is currently write-only — Edge reads every field but heap_size_limit unconditionally, so the mask changes nothing.
3. Engine-flag ordering in AcquireRuntime — the first env in the process fixes the flags permanently, and a later mismatch is now a hard napi_invalid_arg, including the "worker starts first" case.
4. src/guest/abi.rs is hand-maintained wasm32 offsets with no static assertion tying it to the header, and it silently drops guest_heap despite the header's exactly-once ownership contract.
5. GetErrorMetadata(_current) re-enters JS (the source-map callback) on fatal-exception formatting paths that previously only asked for thrown_at.

Smaller: size_t vs uint32_t for the size prefix across descriptors, a success value written into *result_out on a failing profile_start, and the bridge accepting hooks it discards.

Nothing here is an objection to the direction — the ABI is clearly better than what it replaces. Items 1 and 3 are the ones I'd consider release-blocking, since they change observable behaviour outside the API surface being reduced.

[Arshia001]

Folding get_process_memory_info into get_heap_statistics drops two values on QuickJS.

The removed unofficial_napi_get_process_memory_info() reported *array_buffers_out = usage.binary_object_size. The merged snapshot never assigns array_buffer_memory, and edgejs#147's ProcessMethodsMemoryUsageBufferCallback reads it unconditionally:

values[4] = static_cast<double>(heap_statistics.array_buffer_memory);

so process.memoryUsage().arrayBuffers goes from binary_object_size to a hard 0 on the QuickJS provider.

Same hunk, smaller: stats_out->peak_malloced_memory = stats_out->malloced_memory; was deleted, so v8.getHeapStatistics().peak_malloced_memory also becomes 0 there. If reporting 0 is the intended honest answer for both, that is defensible — but it needs to be paired with a consumer that actually respects valid_fields (see the next comment), otherwise callers just read zeros as facts.

[Arshia001]

valid_fields has no consumer, so it doesn't buy the honesty it's designed for.

This is the only place that reports a partial mask, and in edgejs#147 the only field ever tested against it is heap_size_limit:

.memory_limit = (stats.valid_fields & unofficial_napi_heap_stat_heap_size_limit) != 0
                    ? stats.heap_size_limit : fallback_heap_limit,
.malloced_memory = stats.malloced_memory,   // read regardless
.external_memory = stats.external_memory,   // read regardless
...

Every unset QuickJS field is therefore surfaced to JS as a genuine 0 anyway. The mask is worth keeping — it's the right shape — but it needs either a consumer-side helper that folds unsupported fields to a documented sentinel, or a note in the header that callers must check it per field. As-is it's ABI surface that costs 8 bytes and changes no behaviour.

[Arshia001]

Writes a success value into *result_out and then returns failure.

*result_out = unofficial_napi_profile_start_ok;
*profile_out = nullptr;
return napi_generic_failure;

A caller that inspects the out-param before the status — which the two-out-param shape invites — sees "started fine, no session". The V8 path only writes result_out on paths that return napi_ok. Since the contract is "unsupported", leaving *result_out untouched (or defining an explicit unofficial_napi_profile_start_unsupported) would be safer, and matches the header's promise that result_out distinguishes ok from busy.

[Arshia001]

First env in the process permanently fixes the flag string, and a mismatch is now a hard failure.

Two ordering hazards fall out of moving flags into create_env:

• g_runtime.platform is deliberately never torn down (ReleaseRuntime() only decrements), so engine_flags is captured once for the life of the process. A second create_env with any different non-empty flags returns napi_invalid_arg — where the old set_flags_from_string silently no-op'd. That converts "flags ignored" into "environment creation failed", which is more honest but is a behaviour change embedders need to know about.
• The asymmetry is sharper than the comment suggests: if the first env is created with options == nullptr or an empty flag string (any worker via EdgeInitializeNapiEnvCreateOptions, or EnvScope in the test harness), a later env that does pass flags also fails, because engine_flags.size() != engine_flags_length. So in a process that starts a worker before the main env, the main env can't be created at all.

A distinct status (napi_generic_failure with a comment, or an explicit "runtime already initialized" code) plus a header note that flags are honoured only from the first create_env would make this diagnosable. Right now the caller sees napi_invalid_arg and can't tell it apart from a malformed descriptor.

[Arshia001]

size is size_t here and in unofficial_napi_module_hooks (line 764), but uint32_t in every other versioned descriptor.

unofficial_napi_env_create_options (27), unofficial_napi_env_hooks (76), unofficial_napi_heap_statistics (370), and unofficial_napi_bytecode_open_options (566) all use uint32_t size. These two use size_t, which is 4 bytes on wasm32 and 8 on the native host — so the field width of the "how do I validate this descriptor" header differs by target for two of six descriptors.

It happens to work today because src/guest/abi.rs reads all of them with u32_at(&bytes, 0) and only the wasm32 layout crosses that boundary, but it means the same struct has two different prefix layouts depending on where it's compiled, which is exactly what size/version exists to prevent. Please make these uint32_t for consistency.

(Nit in the same area: UNOFFICIAL_NAPI_BYTECODE_OPEN_OPTIONS_VERSION is introduced at 2u with no version 1 having shipped.)

[Arshia001]

Hand-maintained wasm32 struct offsets with nothing tying them to the header.

ENV_CREATE_PREFIX_SIZE = 52, BYTECODE_OPEN_SIZE = 48, MODULE_CREATE_PREFIX_SIZE = 40, ENV_HOOKS_SIZE = 40, plus the per-field offsets below (44/48 for engine flags, 36/40/44/45 for the cache block, 24/28/32/36 for the module payload union) are all derived by hand from include/unofficial_napi.h. I worked through them and they're correct for the current header — but nothing in the build fails if a field is added, reordered, or has its type changed. The failure mode is a silent misread of guest linear memory, which is about the worst diagnostic surface available.

The size/version prefix check helps only for appended fields; it can't catch a reordering or a widened middle field, since size would be unchanged or larger either way.

Two cheap options: emit these constants from a wasm32 static_assert(offsetof(...) == N) header compiled in CI, or generate abi.rs from the same typed inventory the migration plan calls for in step 1. Given the plan explicitly lists "define one typed ABI inventory and generate declarations, bridge registration, and import-conformance checks from it", this file looks like the place that was supposed to be generated.

[Arshia001]

guest_heap (offset 40) is never read, so a guest that sets it leaks the resource.

read_env_create decodes offsets 8–36 and then jumps to 44/48 for the engine flags, skipping the guest_heap pointer entirely. The header is explicit that ownership transfers on a size/version-valid descriptor:

Ownership transfers only after size and version validation proves that this field is present ... It is then released exactly once via napi_host_guest_heap_release.

Today edgejs zeroes the field on wasm32 so nothing leaks in practice, but the bridge is the layer that promises the contract, and it currently promises something it doesn't do. Please either forward it (releasing on every failure path, as the V8 provider does) or explicitly return None when offset 40 is non-zero, so an unsupported transfer fails loudly instead of leaking.

[Arshia001]

_current mode now runs the source-map JS callback on paths that previously never touched it.

Everything after this line — GetErrorSourceLineForStderrImpl() and the full Thrown at: stack format — runs unconditionally for _current, and GetErrorSourceLineForStderrImpl() calls back into state.get_source_map_error_source, i.e. into JavaScript.

In edgejs#147 the thrown_at-only callers were rewritten onto _current:

• FormatUncaughtExceptionForStderr() under --trace-uncaught, which previously called only get_error_thrown_at;
• the out->thrown_at.empty() fallback in TakePendingExceptionInfo().

Both are fatal-exception formatting paths, so this introduces a JS re-entry (and its own possible throw) where there previously was none, plus an unconditional full stack-trace format for callers that wanted a single field.

positions_only already demonstrates the pattern — a third mode, or nullable out-params like module_wrap_get_state uses, would keep "one observation" without making every caller pay for and re-enter the most expensive field.

[Arshia001]

Five of the seven hooks in the table are silently discarded on this target.

snapi_bridge_unofficial_attach_env builds a zeroed unofficial_napi_env_hooks and forwards that, so cleanup_callback, destroy_callback, context_token_assign/unassign_callback, and enqueue_foreground_task_callback never reach the provider. read_env_hooks in src/guest/abi.rs reinforces this — it only decodes offsets 32 and 36 (fatal/OOM) and then let _ = (fatal_callback_id, oom_callback_id); throws those away too.

For fatal/OOM that matches the old set_fatal_error_callbacks(env, nullptr, nullptr) behaviour, and edgejs previously compiled the other four out entirely behind #if defined(EDGE_EMBEDDED_NAPI_PROVIDER). But that #ifdef is removed in edgejs#147, so Edge now builds and sends a complete table on WASIX and gets napi_ok back for hooks that were dropped. That is exactly the "provider may report an unsupported capability" case the surface-reduction plan calls out, and silently succeeding is the one response it rules out.

Please either return a distinguishable status when a non-null unsupported hook is supplied, or at minimum expand the comment to enumerate which four are dropped so the next reader of read_env_hooks doesn't assume the truncation is a decoding bug.