[#890] Persist a compressed schema token before handing it out, and report the ones with no definition

[vharseko]

Fixes #890

CompressedSchema.getAttributeId() registered a new token in the in-memory maps and only then wrote it to the storage. When the write failed the exception was propagated - the operation failed, as it should - but the registration stayed behind, so every later encode of the same attribute description took the lock-free fast path and succeeded. Entries were then written referencing a token whose definition never reached the storage. getObjectClassId() had the same shape.

What it cost

• The entries carrying the lost token do not decode after a restart: the slot is null and decodeAttribute() reports ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN, or - if the lost token was the highest one - adDecodeMap is simply shorter and CopyOnWriteArrayList.get(adId) throws IndexOutOfBoundsException.
• Worse than the entries not decoding: where the lost token was the highest one, the next registration after the restart allocates the same id, so the entries written with it decode as a different attribute - or, for an object class set, as a different set. Nothing is reported; it is indistinguishable from correct data.
• reloadAttributeTypeMaps() walked adDecodeMap by index and dereferenced a padded slot, so a gap brought down reloadMappingsIfSchemaChanged(), which runs on every decode. reloadObjectClassesMap(), getAllAttributes() and getAllObjectClasses() did the same - the last two are what DefaultCompressedSchema rewrites its file from, so a gap stopped it from saving at all.

The fix

The registration is persisted before it is published:

• the element is appended to the decode map first, because storeAttribute() is free to persist the whole content of the compressed schema rather than the single element it is handed - DefaultCompressedSchema.save() rewrites its file from getAllAttributes() - so the element has to be part of it by then. The decode map is not what an encode reaches an id through, so nothing can yet write an entry carrying it;
• the id reaches the encode map - the lock-free path an encode does take - only once the store returned. Withdrawing it after publishing it, as one would first write it, is not enough since Remove per-attribute read lock from CompressedSchema decode path #670 removed the shared lock from the fast path: another thread can read the encode map while the store is still running, and would write an entry with an id about to be withdrawn. aTokenIsPublishedOnlyOnceItIsStored and anObjectClassTokenIsPublishedOnlyOnceItIsStored pin exactly that, and fail on master;
• a store that fails withdraws the appended element - the last one, removed by index under the exclusive lock, so no other id shifts - and the next attempt allocates the id again and stores it. The javadoc of registerAttribute() records what that argument rests on: the lock is reentrant, so an implementation of the store must re-enter neither the encode, the load nor the decode path of the compressed schema.

PersistentCompressedSchema no longer absorbs the IOException of its ASN.1 writing. That catch is defensive - the writer encodes into a ByteStringBuilder, and none of the write methods of the OutputStream it hands out declares IOException, so nothing under the try can raise one today. What makes the withdrawal reachable in a running server is store()'s own catch (Exception), on a storage.write that failed. Absorbing the IOException would nonetheless have let a store that did not happen return normally, and the comment on the catch now says which of the two is which.

Independently of the above, and because a compressed schema written by an older server can carry gaps for other reasons:

• decodeAttribute() and decodeObjectClasses() report a token that is out of range, or below it, as the unknown token it is instead of letting IndexOutOfBoundsException out. In tree this changes the message rather than the exception - Entry.decode() already converted anything unchecked into a DirectoryException through its generic catch, and the stack that catch used to log is kept by tracing in decodeMapGet(). What it does close is the contract of a @PublicAPI class for callers outside the tree. The token is named as the storage holds it, with the id it decodes to, since an all-zero token reads as the id -1 - a value appearing nowhere in the stored data;
• a schema reload carries a null slot over as a gap rather than dereferencing it - dropping it would shift the ids of the elements after it, and would let the next registration hand out an id an already written entry carries;
• saving the whole content skips the gaps rather than failing on them, and the counters DefaultCompressedSchema records with it are taken from the highest token written rather than from the number of records, which a gap makes smaller. Both ends read those counters as "No longer used", but a release old enough to seed from them would otherwise re-issue live tokens.

Testing

CompressedSchemaTestCase (7 tests), run with mvn -Pprecommit verify -pl opendj-server-legacy -Dit.test=CompressedSchemaTestCase:

• on this branch: Tests run: 7, Failures: 0, Errors: 0;
• on master: each fails with the symptom it is written for - NullPointer ... because "ad" is null, the token was handed out before it was stored: 0, the failed registration was left behind expected [2] but found [1] (attribute and object class set alike), ArrayIndexOutOfBounds Index -1 out of bounds for length 0.

Passing on master is not on its own enough for the tests named for the withdrawal, so the two variants that would leave the defect in place were built and run as well:

variant	result
the finally block deleted from registerAttribute() - the registration leaks its decode map element	attributeTokenIsWithdrawnWhenItCannotBeStored:195 fails, the withdrawn id was not allocated again expected [0] but found [1]
registerObjectClasses() publishing the id before storing it, rolling both maps back on failure	anObjectClassTokenIsPublishedOnlyOnceItIsStored:297 fails, the token was handed out before it was stored: 0

The second variant leaves objectClassTokenIsWithdrawnWhenItCannotBeStored green, which is why the ordering of registerObjectClasses() needed a concurrency test of its own rather than the failure path alone.

Note

Reachable on any backend whose write can fail. The JDBC backend makes it ordinary rather than exotic, and for a reason worth recording separately: the entry is encoded inside an already open write transaction (ID2Entry.put() calls encode() within txn), while store() goes through storage.write(), which borrows a second connection from the pool while the first is held. That belongs to #872 / #877, not here.

The load SPI - loadAttribute()/loadObjectClasses(), the sibling of the read path hardened here - still admits a negative or arbitrarily large token from a corrupt store. Pre-existing and untouched by this PR; raised as #897.

[maximthomas]

The fix is right and the ordering argument holds: appending to the decode map first, storing, then publishing to the lock-free encode map is the correct order, and withdrawing on failure so the id is re-allocated is the correct rollback. Format compatibility is fine — the token is in the record, not positional, so skipping gaps on save does not shift ids in either direction.

One major, all in the tests: the withdrawal — the mechanism the PR is named for — is not pinned by anything. Plus a few places where the description claims more than the diff delivers.

The withdrawal is not tested (major)

opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java:184-197, twin at :217-228.

Both tests pass with the finally block deleted from registerAttribute/registerObjectClasses. Take the leak variant — encode map not published on failure, decode element left behind:

1st encode: id 0, adDecodeMap = [ad], store throws
retry:      encode-map miss -> id = adDecodeMap.size() = 1, stores 1, publishes 1

attributeStoreCount == 2 holds. encodedToken is read from the retry's builder, so it is 1, and storedAttributes.containsKey(1) holds. Decoding 1 returns ad. All seven assertions pass in both tests, and neither ever reads a decode-map size.

That leaked slot is a definition DefaultCompressedSchema.save() would persist although its store never succeeded — exactly what remove(size() - 1) exists to prevent.

// closes it: the leak variant yields 1
assertEquals(encodedToken, 0);
// stronger
assertEquals(compressedSchema.savedAttributeTokens(), singletonList(0));

The object-class ordering is untested (minor)

CompressedSchemaTestCase.java:87-93 vs :79-83. The double gates only the attribute hook:

protected void storeAttribute(...)      { attributeStoreCount++;   awaitIfGated(); failIfRequested(); ... }
protected void storeObjectClasses(...)  { objectClassStoreCount++;                 failIfRequested(); ... }

aTokenIsPublishedOnlyOnceItIsStored drives encodeAttribute only, so registerObjectClasses's ordering is exercised through the failure path alone.

Rewriting it as publish-first-then-store, with the encode entry also rolled back in the finally, keeps all 6 tests green. That variant is unsafe for the reason the PR body itself gives: since #670 the fast path is lock-free, so another thread can read the published id while the store is still running and write an entry carrying an id about to be withdrawn. Caught for attributes, not for object class sets.

(The naive revert does not stay green — a stale ocEncodeMap entry makes the retry fast-path and never store, failing :223. It is specifically the publish-and-roll-back variant that slips through.)

Call awaitIfGated() from storeObjectClasses and mirror the concurrency test.

The ordering test proves itself with a wall clock (minor)

CompressedSchemaTestCase.java:253-260, worker at :278-290.

assertTrue(started.await(30, SECONDS));
fail("the token was handed out before it was stored: " + concurrent.get(500, MILLISECONDS));

The worker's first statement is started.countDown() — before new ByteStringBuilder(), before encodeAttribute. The latch proves the task body began, not that it reached the lock-free adEncodeMap.get(ad).

A regressed publish-first build normally fails here, but if the worker stalls 500 ms between the countdown and that read, the timeout arrives anyway and the regression is recorded as a pass. False pass only — never a false failure, since a correct build parks the worker in exclusiveLock.lock() until leaveStore.countDown(). With the previous point, this is the only test pinning the central invariant for either half.

Capture the worker's Thread and poll until getState() is BLOCKED/WAITING instead of timing out.

PersistentCompressedSchema's new catch cannot fire (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java:113-124, :139-148.

Nothing in either try throws IOException. ASN1.getWriter(ByteStringBuilder) resolves to getWriter(builder.asOutputStream(), ...), whose root and every substream is ByteStringBuilder.OutputStreamImpl — its write() overrides do not declare IOException at all. store() declares DirectoryException. The catch compiles only because the ASN1Writer interface declares it.

So the change is defensive-only, which is fine — but the comment is wrong:

// Reported rather than absorbed: the caller takes a store that returned for a definition
// that reached the tree, and an entry written with a token that never did cannot be decoded
// once the server is restarted.

On this path store() was never invoked and nothing reached the tree. Worth stating plainly: the withdrawal is reachable in a real server through store()'s pre-existing catch (Exception) -> DirectoryException on a storage.write failure, not through this new catch.

The IndexOutOfBoundsException rationale is overstated (minor)

The description says the unchecked exception escaped "on a read path written for DirectoryException". org/opends/server/types/Entry.java:3483 opens a try that encloses both decodeObjectClasses (:3533) and both decodeAttributes calls (:3539, :3541), and ends in:

catch (DirectoryException de) { throw de; }
catch (Exception e) {
  logger.traceException(e);
  throw new DirectoryException(getServerErrorResultCode(), ERR_ENTRY_DECODE_EXCEPTION.get(...), e);
}

Entry.java:3688 and :3585 are the only production callers repo-wide. The conversion already existed; the net in-tree change is the message text, minus that traceException stack. Hardening the contract for out-of-tree callers of a @PublicAPI class is real and worth doing — it just is not what is claimed.

Checked the inverse too, and it is clean: bulk readers (ID2Entry, verify, rebuild, import/export, backup) do not start silently skipping corrupt entries, because they already saw a DirectoryException.

Consider keeping a traceException on the new path.

The load SPI still admits a negative or huge token (minor, pre-existing)

opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java:643-659 and :748-765 — untouched by this PR, raised because it is the sibling of what decodeMapGet just hardened.

if (id < mappings.adDecodeMap.size()) { mappings.adDecodeMap.set(id, ad); }
else { while (id > mappings.adDecodeMap.size()) { mappings.adDecodeMap.add(null); } ... }

decodeId folds every byte and returns id - 1, with no length cap and no sign check. From a corrupt store:

• key 0x00 gives id == -1; the guard passes and set(-1, ad) throws, escaping initializeSchema() as a RuntimeException (DefaultCompressedSchema:147) or as a StorageRuntimeException (RootContainer:145-152);
• key 0x7FFFFFFF gives id == 2147483646; the else branch pads a CopyOnWriteArrayList one element at a time, unbounded, holding exclusiveLock and — for the pluggable backend — inside RootContainer.open's write transaction. Backend open hangs silently.

Follow-up issue rather than this PR.

Nits

• Unknown-token tests assert nothing about the exception: CompressedSchemaTestCase.java:299-308, :317-326 — catch (final DirectoryException expected) with no ResultCode or message-id check, so drift away from ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN is invisible. No live false pass today; decodeAttribute has one throw site.
• Boundary coverage: -1/0/7 are probed against an empty decode map, so token == size and token > size on a populated one are never exercised. Same code path, so a nit rather than a hole.
• The decode map became shrinkable: CompressedSchema.java:514/529, :570/574 do size() then get() on a CopyOnWriteArrayList that remove(size() - 1) can now shrink — the split decodeMapGet's own comment calls unsafe. Unreachable in-tree (getAll* runs only under exclusiveLock via save()); reachable from a subclass, since the class is mayExtend = true.
• Withdrawal removes by index, not identity: "every append is made under the exclusive lock" covers cross-thread serialisation but not same-thread re-entrancy. A subclass whose storeAttribute() re-enters loadAttribute() and then throws removes the wrong element. Worth a javadoc line saying an implementation must not re-enter the encode or load path.
• Message names the database: ERR_COMPSCHEMA_CANNOT_STORE_EX_459 reads "...in the database: %s" but now also guards in-memory ASN.1 encoding. Argument count is correct.
• Token reported is the decoded id: ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN.get(adId) reports -1 for an all-zero on-disk token, a value appearing nowhere in the stored data.
• adCounter/ocCounter now under-report: DefaultCompressedSchema.save() counts emitted records, so gaps make the trailing integers smaller than maxId + 1. Harmless here — both ends read them as "No longer used" — but an older release seeding a counter from them would re-issue live tokens. Untested against an old release.
• Inconsistent final: CompressedSchema.java:506 private List<AttributeDescription> adDecodeMap vs :564 private final List<Map<ObjectClass, String>> ocDecodeMap.

[vharseko]

Thank you - the major is real, and I could reproduce your trace exactly. All eight points are addressed in 8ab8047, and the PR description is corrected where you showed it claimed more than the diff delivers.

The withdrawal is not tested (major) - confirmed and closed

Your trace holds line for line: failIfRequested() throws before storedAttributes.put(), so "nothing was persisted" holds; the encode map was never published, so the retry allocates size() == 1; encodedToken is read from the retry's builder, so containsKey(1) holds and decodeAttribute(1) returns ad. All seven assertions pass on the leak variant, in both tests.

Both suggestions taken, CompressedSchemaTestCase.java:195 and :236:

assertEquals(encodedToken, 0, "the withdrawn id was not allocated again");
assertEquals(compressedSchema.savedAttributeTokens(), Collections.singletonList(0),
    "the whole content still holds the element of the failed registration");

Rather than argue it, I built the two variants and ran them:

variant	result
the finally deleted from registerAttribute()	attributeTokenIsWithdrawnWhenItCannotBeStored:195 - the withdrawn id was not allocated again expected [0] but found [1]
registerObjectClasses() publishing before storing, rolling both maps back	anObjectClassTokenIsPublishedOnlyOnceItIsStored:297 - the token was handed out before it was stored: 0

The object-class ordering is untested (minor) - closed

awaitIfGated() is now called from storeObjectClasses() too, and anObjectClassTokenIsPublishedOnlyOnceItIsStored mirrors the attribute test. Your reading of which variant slips through is exactly right, and the run above confirms both halves of it: the publish-first-with-rollback variant fails the new test while leaving objectClassTokenIsWithdrawnWhenItCannotBeStored green - which is why the failure path alone was not enough.

The ordering test proves itself with a wall clock (minor) - closed

Replaced with awaitParkedOnTheLock(): the task publishes its Thread into an AtomicReference and the test polls until the state is WAITING/BLOCKED, checking isDone() first so a regression fails immediately with the id it was handed rather than after the deadline. That is what produced the the token was handed out before it was stored: 0 line above, in 15 s of wall clock for the whole class.

PersistentCompressedSchema's new catch cannot fire (minor) - confirmed

Checked against the byte code of the pinned opendj-core:

ASN1.getWriter(ByteStringBuilder) -> ByteStringBuilder.asOutputStream() -> getWriter(OutputStream, 32768)

final class ByteStringBuilder$OutputStreamImpl extends OutputStream {
  public void close();  public void write(byte[]);  public void write(byte[], int, int);  public void write(int);
}

No IOException on any of them. The comment is rewritten to say the catch is defensive, that the reachable path is store()'s own catch (Exception) on a failed storage.write, and what would still be wrong about absorbing it - rather than claiming the tree was written.

The IndexOutOfBoundsException rationale is overstated (minor) - confirmed

Entry.decode()'s generic catch already converted it, and Entry.java:3585 / :3688 are the only production callers repo-wide. The description now says so: in tree this changes the message, not the exception; what it closes is the @PublicAPI contract for callers outside the tree. logger.traceException(e) is back, in decodeMapGet()'s catch, so the stack the generic catch used to log is kept for exactly the newly converted case and the common path keeps its single read.

The load SPI still admits a negative or huge token (minor, pre-existing) - #897

Agreed it is a follow-up, raised as #897. One correction that makes the second case worse than "unbounded padding": the decode map is a CopyOnWriteArrayList, so each add(null) copies the whole backing array - two billion appends, quadratic, under exclusiveLock and inside RootContainer.open()'s write transaction. The backend does not open slowly, it never opens.

Nits

• Unknown-token tests assert nothing about the exception - closed: assertMessageIs() compares resourceName() + "-" + ordinal(), so drift away from ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN / ERR_COMPRESSEDSCHEMA_UNKNOWN_OC_TOKEN fails the test.
• Boundary coverage - closed: each token set now runs against a decode map populated with two definitions as well as an empty one, so token == size and token > size are both exercised.
• The decode map became shrinkable - closed: getAllAttributes()/getAllObjectClasses() read through decodeMapGet() in both hasNext() and next(), so a shrink ends the iteration instead of throwing.
• Withdrawal removes by index, not identity - closed, and worth stating a degree stronger than a wrong element. Re-entering the decode path is the sharper case: decodeAttribute() calls reloadMappingsIfSchemaChanged(), which replaces this.mappings wholesale under the same reentrant lock, having built the new map from the old one including the element just appended. The finally then removes it from a map that is no longer live, so the registration is not withdrawn at all - the leak this PR exists to prevent, plus the id is now taken. Unreachable in tree (getMappings() does not reload; save() and the pluggable store never decode), so the javadoc of registerAttribute() now names all three paths a store must not re-enter.
• Message names the database - left as it is, and I would rather say why than change it quietly: ERR_COMPSCHEMA_CANNOT_STORE_EX is shared with store(), where "in the database" is accurate, and a separate id means a new ordinal across nine locale bundles for a catch that cannot fire. Happy to add one if you would rather have it.
• Token reported is the decoded id - closed: tokenInMessage() names the token as the storage holds it with the id it decodes to, so an all-zero key now reads 0x00 (id -1) instead of a bare -1.
• adCounter/ocCounter now under-report - closed: counterAfter() takes the counter from the highest token written rather than from the record count. With no gaps it is the value it was before (ids 0..N-1, tokens 1..N, so N+1); with a gap it names the token after the highest one, which is what an older release would need to seed from.
• Inconsistent final - closed.

Untouched from the last round and worth repeating for the record: the JDBC second-borrow note stays under #872 / #877, not here.