Skip to content

Keep module context data when a module's first hook returns none - #138

Merged
fushouli merged 1 commit into
msp-2506from
fix-nil-module-context
Sep 23, 2026
Merged

fushouli merged 1 commit into
msp-2506from
fix-nil-module-context

Conversation

@zwl-williamzha

Copy link
Copy Markdown

Problem

After the v4.9.0 sync (#137), moduleContexts.put stores whatever a module's first hook returns, including nil:

existingCtx, ok := mc.ctxs[moduleName]
if !ok {
    mc.ctxs[moduleName] = mCtx   // may be nil
    return
}
existingCtx.SetAll(mCtx.GetAll()) // no-op on a nil receiver

Once nil is stored, every later stage's SetAll does nothing, so all context data the module writes afterwards is silently dropped. v3 handled this: it replaced a nil entry (ok && existingCtx != nil). Upstream's move to *ModuleContext in prebid#4603 lost that check.

Impact on MSP stage: ad_cache's entrypoint hook doesn't set a context, so the placement, user and impression ids set in processed_auction never reached later stages. Because of that, ad-cache ranking, dynamic bidder floors, FB demote, the fraud-user check and last-bid-floor writes were all skipped, and msp_app_level_fill_counter / msp_bidder_fill_counter stopped. test_mode has the same bug for test traffic. Prod is unaffected because it's still on the pre-sync build.

Fix

Replace the stored entry when it is nil (if !ok || existingCtx == nil), which restores v3's behavior.

Testing

  • New hooks/hookexecution/context_test.go:
    • TestModuleContextsPutAfterNil fails without the fix and passes with it.
    • TestModuleContextsPutMergesAndKeepsNilHarmless covers normal merging, including a nil result after data is stored.
  • go test -race ./hooks/... passes.

The same fix should go to upstream prebid-server, since it affects any v4 host with a module like this.

🤖 Generated with Claude Code

v4's moduleContexts.put stores whatever the first hook returns, so a
nil context gets stored for the module. Every later SetAll on that nil
pointer is a no-op, and all data from later stages is silently lost.
v3 replaced a nil entry instead (ok && existingCtx != nil), and the
move to *ModuleContext in prebid#4603 dropped that.

In MSP, ad_cache's entrypoint hook returns no context, so on stage its
placement/user/imp ids never reached the bidder request or auction
response stages: ad-cache ranking, dynamic floors and FB demote were
skipped, and msp_app_level_fill_counter stopped.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@fushouli
fushouli merged commit 11a3a01 into msp-2506 Sep 23, 2026
@fushouli
fushouli deleted the fix-nil-module-context branch September 23, 2026 21:26
fushouli pushed a commit that referenced this pull request Sep 28, 2026
Reverts #137 and #138, which fixed a v4-only regression in the module
context code #137 brought in and does not apply without it. The tree is
identical to ad5ae87 (#135), the last commit before the sync.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants