166 lines
10 KiB
Markdown
166 lines
10 KiB
Markdown
# Task 6 — the five reviews' P0 findings, reconciled
|
||
|
||
The remediation design this wave executed never referenced a review id (`grep -c` over it: zero).
|
||
It took one theme from the five reviews — five adapters ship and do not run — and Waves 0–6 executed
|
||
that theme. So "how much of the reviews is reflected" had never been measured. This is the first
|
||
measurement, and then the work that followed it.
|
||
|
||
Three read-only audits ran in parallel, one per review family, each instructed to treat a passing
|
||
test as evidence only when it exercises the real composition. Their headline claims were then
|
||
re-checked by hand before being acted on; one was wrong and is recorded as such below.
|
||
|
||
## Where the P0 set stood when measured
|
||
|
||
| Review | P0 | CLOSED | PARTIAL | OPEN |
|
||
| --- | ---: | ---: | ---: | ---: |
|
||
| jpa | 6 | 5 | 1 | 0 |
|
||
| messaging | 5 | 2 | 3 | 0 |
|
||
| graphql | 2 | 1 | 1 | 0 |
|
||
| notification | 12 | 1 | 11 | 0 |
|
||
| mongodb | 9 | 0 | 9 | 0 |
|
||
| **total** | **34** | **9** | **25** | **0** |
|
||
|
||
Nothing was OPEN: every P0 had been worked. What the audits found instead, in three independent
|
||
voices, was the same shape — **the implementation is substantially real and the gate under it is
|
||
thin**. Six notification adapter classes were referenced by zero tests. Both Mongo executors were
|
||
referenced by zero tests. Every messaging real-broker test skips silently without Docker. And the
|
||
Compose matrix, the strongest evidence this repository produces, runs in no workflow.
|
||
|
||
## What was fixed, and how each was proven
|
||
|
||
### NTF-004 / JPA-004 — the completion write was not fenced
|
||
|
||
Two audits reached this independently from different reviews, which is why it was taken first.
|
||
|
||
The claim is fenced (a single `FOR UPDATE SKIP LOCKED` CTE that bumps `lease_fence`) and the renew
|
||
is fenced. The *completion* was `findById → mutate → saveAndFlush` with no owner or fence predicate.
|
||
A worker whose lease expired during the provider call — the one stretch the platform deliberately
|
||
spends outside a transaction — came back and wrote its outcome over the row a new holder had already
|
||
claimed. The `@Version` column does not stop that: it detects a concurrent edit, not a superseded
|
||
writer, and the late worker's read is recent enough to win.
|
||
|
||
Fixed with conditional statements in the same idiom as the renew, `saveHeldBy`/`transitionHeldBy` on
|
||
the port returning empty when the lease is gone, and all four branches of `applyNextAction` moved
|
||
onto them. Losing the lease is not an error: the new holder owns the job and will record its own
|
||
outcome.
|
||
|
||
`@Modifying(clearAutomatically = true)` on both, because a native update bypasses the persistence
|
||
context and the immediate re-read would otherwise be served the values it just replaced — a trap one
|
||
of the audits had found elsewhere in this same platform.
|
||
|
||
**Proof:** two real-PostgreSQL cases in the JPA contract lane. A superseded holder's completion
|
||
matches zero rows and the live holder's state is untouched; the current holder's completion writes.
|
||
The second exists because without it the first is satisfied by a statement that matches nothing ever.
|
||
|
||
### NTF-012 — the SSRF guard had no callers
|
||
|
||
`requireExternallyRoutable` refuses the cloud metadata service, RFC 1918, link-local, IPv6 local,
|
||
userinfo disguise and multi-answer DNS. It had an eight-case test suite, all green, and **one
|
||
occurrence in the repository: its own definition.** The two sites it was written for —
|
||
`WebhookSubscription` and `SesProviderProperties` — still called `requireSecureOrLoopback`, which
|
||
reads the scheme and nothing else.
|
||
|
||
Both now call it. The new test goes through the constructors rather than the helper, because testing
|
||
the helper again is exactly what failed to catch this.
|
||
|
||
What is not closed: `allowLoopback` is true, so a user-supplied target naming `localhost` still
|
||
passes. Closing it means the allowance becomes a decision the caller states, and the caller does not
|
||
exist — WEBHOOK has no `ProviderRuntimeAssembler`, so a webhook profile refuses to boot and nothing
|
||
in production constructs the record. Writing the policy now would mean choosing its default with no
|
||
caller to check it against. Recorded in the code at the call site.
|
||
|
||
### NTF-001 — the documented callback switch broke startup
|
||
|
||
`CallbackRequestFactory` is a constructor argument of the MVC controller, the WebFlux handler and the
|
||
WebFlux configuration, and was produced by no production code — the only instantiation was in a test.
|
||
So `APP_NOTIFICATION_PLATFORM_CALLBACKS_ENABLED=true`, a key in the env registry and in the
|
||
configuration reference, did not enable callbacks; it failed the boot on an unsatisfied dependency.
|
||
|
||
The beans now exist, conditioned on the same switch. The missing `trusted-proxies` setting came with
|
||
them, defaulting to empty — with no entry, forwarded headers are not believed, because honouring
|
||
them unconditionally lets any caller choose the URL its own signature is checked against. Registered
|
||
in `application.yml`, the env registry, `.env.example` and the configuration reference; both
|
||
notification gates pass.
|
||
|
||
### MNG-007 — a health view Actuator could not read, and a reactive half nothing built
|
||
|
||
`MongoPlatformHealthIndicator` computed topology mismatch, secondary availability and a bounded
|
||
detail map, and implemented neither `HealthIndicator` nor `HealthContributor`. The adapter carries no
|
||
Actuator dependency and should not: the delivery platform had already established the shape, where
|
||
the adapter computes the facts and the composition root maps them onto `Health`. Done the same way.
|
||
|
||
Separately, every reactive class in the leaf — executor, consistency binder, session factory, cursor
|
||
guard — was declared by no configuration. They shipped and no configuration could construct them.
|
||
Now wired in a nested configuration conditioned on a `ReactiveMongoTemplate` *bean*, not just the
|
||
class: the class is on the compile classpath unconditionally, so a class condition alone would try to
|
||
build the reactive path in a servlet-only deployment and fail a startup for a capability nobody asked
|
||
for. Both the positive and the negative case are asserted.
|
||
|
||
### MNG-005 / MNG-023 — a deadline that only produced a report
|
||
|
||
The blocking executor compared elapsed time to the declared timeout *after* the callback returned,
|
||
and said so in its own comment: a Java callback cannot be interrupted mid driver call. That is an
|
||
overrun report, not a deadline.
|
||
|
||
The scoped API is the narrowed surface where the number can actually be sent, so every method taking
|
||
a `Query` or an `Aggregation` now carries it as `maxTimeMS`, which the server enforces. `insert` has
|
||
no query to attach it to. `Duration.ZERO` is refused, because zero means "no limit" to the server and
|
||
accepting it would turn a misconfiguration into an unbounded operation.
|
||
|
||
The raw escape hatches (`rawOperations()`, `executeInternal(...)`) are genuinely used inside the
|
||
platform by the geospatial, atomic and bulk operations, and their callers live in sibling packages,
|
||
so package-private cannot express the rule. It is enforced as a boundary from the composition root —
|
||
the only place that sees both the platform and everything consuming it. The rule was falsified by
|
||
widening its scope until it fired, and a second case asserts the platform still uses them, so the
|
||
rule cannot pass by the hatches having been deleted.
|
||
|
||
### MNG-006 — a guard that compared a declaration against nothing
|
||
|
||
`LocalDateTimeMappingGuard` was constructed `withoutConverters()` and then asked to validate the
|
||
manifest. It could only ever reject `LOCAL_DATE_TIME_WITH_REGISTERED_CONVERTER`: a deployment that
|
||
*had* registered the named converter was rejected exactly as loudly as one that had not, so the check
|
||
that exists to distinguish those two cases could not tell them apart. It now reads the converters the
|
||
deployment actually registered, and a test asserts the same manifest passes with the converter and
|
||
fails without it.
|
||
|
||
### MNG-008 — a promotion gate that required five of the six categories it declares
|
||
|
||
`MongoAdvancedPromotionEvidence.REQUIRED` listed six; `MongoAdvancedPromotionGate.verify()` required
|
||
five. `migration` was missing, so a promotion could pass with no migration evidence at all.
|
||
|
||
### GQL-002 — the batch policy applied to nothing
|
||
|
||
`GraphQlBatchLoaderRegistrar` carried the chunking, the budget and the request scope, was unit
|
||
tested, and was declared by no configuration — the only file mentioning it was itself. A field
|
||
resolving through `@BatchMapping` or a `DataLoader` met none of it. The chain
|
||
(`BatchPolicyRegistry → DataLoaderFactory → BatchLoaderRegistrar`) is now assembled by the platform.
|
||
The batch ceiling reuses `maximumPageSize` rather than adding a setting: both answer how many rows
|
||
one downstream call may ask for, and a batch limit above the page limit would let one request fan out
|
||
past the bound it already accepted.
|
||
|
||
## One thing an audit got wrong, and one fix that was reverted
|
||
|
||
The mongodb audit reported that the UUID axis is "declaration-only" and mapped onto the driver
|
||
nowhere. It is mapped — `MongoClientSettingsFactory` calls `.uuidRepresentation(...)`. Verified
|
||
before acting.
|
||
|
||
Acting on the adjacent concern — the manifest and the profile describing the same fact independently
|
||
— a startup check was written to refuse a disagreement between them. Writing its test showed the
|
||
disagreement cannot occur: `MongoUuidRepresentation` has two values, only `STANDARD` is writable, and
|
||
an existing check already refuses the other. **A guard for a state that cannot arise is the same
|
||
"declaration nothing checks" this wave has been removing**, so it was reverted rather than kept with
|
||
an unfalsifiable test.
|
||
|
||
## What remains, and why
|
||
|
||
- **NTF-012 loopback residue** — belongs with the change set that gives WEBHOOK an assembler.
|
||
- **MSG-015** — the application-owned port and anti-corruption bridge between `application-core` and
|
||
the messaging platform. The review names it and specifies the target shape; it is a runtime wiring
|
||
change, and the review itself says the physical work is a separate change set.
|
||
- **MSG-003/004/005 residue** — on code no deployment can execute: the platform's outbox and inbox
|
||
migrations are applied only by tests, the production Kafka transport is publish-only, and Rabbit
|
||
declares no transport bean.
|
||
- **P1 and P2 — 100 findings, never audited.** They were not in this remediation's scope and their
|
||
state is unknown. Saying so is the honest position; the P0 audit took three parallel agents and
|
||
the P1/P2 set is three times larger.
|