bedrock-kv / bedrock / 72847101a379f57434cfb369d4b1a55a6e8173b8
73%

DEFAULT BRANCH: develop
Ran
Jobs 2
Files 215
Run time 1min
Badge
Embed ▾
README BADGES
x

If you need to use a raster PNG badge, change the '.svg' to '.png' in the link

Markdown

Textile

RDoc

HTML

Rst
coverage: 68.76% (-0.004%) from 68.764%
72847101a379f57434cfb369d4b1a55a6e8173b8

push

github

web-flow 
Fix waitlisting with nil reply_fn in Olivine Reading (bedrock-66h) (#89)

## Bug

`handle_version_too_new` in
`lib/bedrock/data_plane/materializer/olivine/reading.ex` waitlisted a
request whenever `wait_ms > 0`, even when `opts[:reply_fn]` was `nil`.
When `notify_waiting_fetches` later processed such an entry,
`execute_fetch_request(context, nil, ...)` invoked
`execute_get`/`execute_get_range`, whose
`do_now_or_async_with_reply(nil, fun)` ran the fetch **synchronously**
and returned `{:ok, value}` — which then matched the unguarded `{:ok,
pid}` clause in `execute_fetch_request` and crashed in `add_active_task`
via `Process.monitor(value)`:

```
** (ArgumentError) errors were found at the given arguments:
  * 2nd argument: not a pid
    :erlang.monitor(:process, "value1")
    lib/bedrock/data_plane/materializer/olivine/reading.ex:348: add_active_task/2
```

Same bug class as PR #79, one layer down.

## Fix (option a: reject waitlisting without a reply_fn)

The waitlist's only delivery mechanism **is** `reply_fn` — a caller that
supplied none could never be notified of the eventual result, and a
`nil` entry would also crash `notify_waitlist_shutdown` and
`execute_fetch_request`'s error branch, which call `reply_fn.(error)`
unconditionally. So `handle_version_too_new` now only waitlists when
`is_function(reply_fn, 1)`; otherwise the caller gets `{:error,
:version_too_new}` immediately, exactly as if `wait_ms` had not been
set. The sole production caller (Olivine `Server.handle_call` for
`:get`/`:get_range`) always injects a `reply_fn`, so no legitimate path
is affected. This was preferred over guarding the `{:ok, pid}` clauses
(option b), which would have papered over a nonsensical waitlist entry
rather than preventing it.

## `{:waitlist_timing, key}` leak

The process-dictionary write at waitlist time was never read or deleted
anywhere — a pure leak. It was evidently meant to feed wait-duration
telemetry when the waitlisted request was later processed,... (continued)

5386 of 7833 relevant lines covered (68.76%)

1974.58 hits per line

Coverage Regressions

Lines Coverage ∆ File
3
85.0
 -7.5% lib/bedrock/data_plane/materializer/basalt/pulling.ex
1
95.65
 -4.35% lib/bedrock/data_plane/materializer/telemetry.ex
Jobs
ID Job ID Ran Files Coverage
1 72847101a379f57434cfb369d4b1a55a6e8173b8.1 215
68.68
 GitHub Action Run
2 72847101a379f57434cfb369d4b1a55a6e8173b8.2 215
68.75
 GitHub Action Run
Source Files on build 72847101a379f57434cfb369d4b1a55a6e8173b8